multi: replace dcrutil usages with stdaddr

[itswisdomagain]

No description provided.

[chappjc]

I'm wondering if it's best to go through an address like we're presently doing to get p2shScript for NewTxOut, or if we should build the payment/output script manually.

contractHash160 := stdaddr.Hash160(contractScript)
// HASH160 <20-byte script hash> EQUAL
p2shScript, err := txscript.NewScriptBuilder().
    AddOp(txscript.OP_HASH160).
    AddData(contractHash160).
    AddOp(txscript.OP_EQUAL).
    Script() // txscript.PayToScriptHashScript()

Basically the reverse of txscript.ExtractScriptHash.

I will say that it's probably less likely to break going through an address though. What do you think @davecgh about building a MsgTx given a redemscript to which a p2sh output should pay? Keep using NewAddressScriptHashV0 and PaymentScript? The actual address is otherwise unused.

[davecgh]

I'd stick with the address here. I expect that we're going to move to blake256 hashes for the script hash at some point (along with a new script version) because 80-bit security is starting to get near the realm of being too low. Using the address will allow you to just do stdaddr.NewAddressScriptHashV1(...) (once it exists of course) and everything else would just work (assuming you also update it to use the returned version for the output too ofc).

[davecgh]

I'd also suggest changing all instances that create tx outputs such as txOut below to use a helper function that also takes the script version:

e.g.

// newTxOut returns a new transaction output with the given parameters.
func newTxOut(amount int64, pkScriptVer uint16, pkScript []byte) *wire.TxOut {
	return &wire.TxOut{
		Value:    amount,
		Version:  pkScriptVer,
		PkScript: pkScript,
	}
}

p2shScriptVer, p2shScript := scriptAddr.PaymentScript()
txOut := newTxOut(int64(contract.Value), p2shScriptVer, p2shScript)
baseTx.AddTxOut(txOut)

This will help ensure the code is ready for new version scripts.

[chappjc]

@itswisdomagain In a different PR I've begun updates for meticulous script version checking and passing. It involves modifying a bunch of types and exported function signatures, so you can defer broader version related changes. Definitely do use version when the scope is limited (like this function), but don't worry about the whole code base in this regard yet.

[chappjc]

@davecgh Suggestion taken regarding wire.TxOut construction, but I forgot to ask at the time of your suggestion why the wire.NewTxOut constructor doesn't get updated instead? Is that planned?

[chappjc]

The script version pertains to the previous output's pkScript (the p2sh contract output). We've already validated the contract output at this point, and we only support v0 presently, so we can just hard-code 0 for now.

However, to support multiple script versions, we'd really want to tell findRedemptionsInTx the script version of each of the outputs we're giving it to check. In addition, each findRedemptionReq could include the script version obtained via msgTx.TxOut[vout].Version in queueFindRedemptionRequest, and the scan would skip over any outputs with an unexpected script version. But both things would need to be done to support multiple script versions, not just 0.

We don't have to do this now, but does that make sense?

[itswisdomagain]

Makes sense, for the most part.

the scan would skip over any outputs with an unexpected script version

If the prevOut's script version were available in tx.Vin[i], we can just ignore inputs with script versions different from the script version of the contract output whose redemption is being searched for. Skipping inputs whose sig script returns ErrUnsupportedScriptVersion will work fine as well.

Can just do that now.

EDIT:
Err, since we'd be passing the contract output's script version for each input being scanned, we might encounter another error trying to tokenize an input sig script whose prev out's script version is different from the contract output's script version. Can deal with that when other non-0 script versions are added.

[chappjc]

Re the prev out's script version, that was one part of my suggestion, but I think with the tx.Vin[i].{Vout,Txid} check in the loop in findRedemptionsInTx, there's no need to pass version in contractOutpoints too I believe.

[chappjc]

Oh, no I see what you're saying. We do need to catch such an error because even now there can be non-0 scripts on the chain.
On the other hand, it wouldn't have passed an audit, so it should not happen if the Vout and Txid match.

[chappjc]

For reference when we look back at this diff: decred/dcrd@06cd8b0

[chappjc]

@vctt94 This should fix the hang at WaitForShutdown. decred/dcrd@c9521b4
I'll test this to be sure, but just FYI.

[chappjc]

Confirmed the fix works. Now when using dcrd's RPC port instead of dcrwallet's I'm getting dcrwallet.Version response missing 'dcrwalletjsonrpcapi' as expected instead of a hang:

[chappjc]

Need pubkey hash address (NewAddressPubKeyHashEcdsaSecp256k1V0). Previously we were relying on the Address method in the return to canonicalize to pkh, but that's no longer the case. Thus we have to make the address the correct type here.

-       addr, err := stdaddr.NewAddressPubKeyEcdsaSecp256k1V0Raw(pubKeyBytes, a.keyParams)
+       addr, err := stdaddr.NewAddressPubKeyHashEcdsaSecp256k1V0(stdaddr.Hash160(pubKeyBytes), a.keyParams)
        if err != nil {
-               log.Errorf("error creating new AddressSecpPubKey: %v", err)
-               return "", fmt.Errorf("error encoding fee address")
+               log.Errorf("Failed to create creating new pubkey hash address: %v", err)
+               return "", fmt.Errorf("error encoding fee address: %w", err)
        }

[chappjc]

Also please apply the following diff to server/auth/registrar.go, otherwise it's difficult to trace this issue down:

diff --git a/server/auth/registrar.go b/server/auth/registrar.go
index 88f0dd87..0ffda883 100644
--- a/server/auth/registrar.go
+++ b/server/auth/registrar.go
@@ -4,6 +4,7 @@
 package auth
 
 import (
+       "errors"
        "fmt"
        "time"
 
@@ -11,6 +12,7 @@ import (
        "decred.org/dcrdex/dex/msgjson"
        "decred.org/dcrdex/dex/wait"
        "decred.org/dcrdex/server/account"
+       "decred.org/dcrdex/server/asset"
        "decred.org/dcrdex/server/comms"
 )
 
@@ -196,6 +198,10 @@ func (auth *AuthManager) handleNotifyFee(conn comms.Link, msg *msgjson.Message)
 func (auth *AuthManager) validateFee(conn comms.Link, acctID account.AccountID, notifyFee *msgjson.NotifyFee, msgID uint64, coinID []byte, regAddr string) bool {
        addr, val, confs, err := auth.checkFee(coinID)
        if err != nil || confs < auth.feeConfs {
+               if err != nil && !errors.Is(err, asset.CoinNotFoundError) {
+                       log.Warnf("Unexpected error checking fee coin confirmations: %v", err)
+                       // return wait.DontTryAgain // maybe
+               }
                return wait.TryAgain
        }
        var msgErr *msgjson.Error

[chappjc]

All simnet trade disruption tests in trade_simnet_test.go pass