redeemmultisigout: assigns fee below min allowed

[norwnd]

I've been playing with redeemmultisigout (and multi-sigs in general), when I'm trying to broadcast 2 of 2 signed transaction via https://testnet.dcrdata.org/decodetx I get:

Error: -1: rejected transaction 602702108225c89e5e9d050c4766858adc1cd53d42ebdefa77a3ea7c058b74dd: regular transaction 602702108225c89e5e9d050c4766858adc1cd53d42ebdefa77a3ea7c058b74dd pays a fee of 2190 atoms which is under the required fee of 2540 atoms for a 254-byte transaction

it seems redeemmultisigout assigns very low fee for such transaction ?

Partially signed transaction (1 of 2) I get from redeemmultisigout looks like this:

0100000001da72d6f35c9cc3aa248341885fa9526eecb3f45a4b9e98d9ed61a0bd086e952c0000000000ffffffff01924500000000000000001976a91412f1e1fb75d8ba8b0d7fa445d5125630aba3c69988ac000000000000000001204e00000000000000000000ffffffff48475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae

while fully signed (2 of 2) gets bigger and looks like this:

0100000001da72d6f35c9cc3aa248341885fa9526eecb3f45a4b9e98d9ed61a0bd086e952c0000000000ffffffff01924500000000000000001976a91412f1e1fb75d8ba8b0d7fa445d5125630aba3c69988ac000000000000000001204e00000000000000000000ffffffff914730440220597473ee2d1bb3581b7327c85f9245d395d339a5caf3ff65e7957d97e9927821022065b97b591c5fb9406a4b87b8424195690bdf152649dbb02c7a6b6636cfea4f2c0100475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae

I've also tried to publish fully signed transaction with dcrwallet, it looks like it accepted the request but didn't actually publish this txn:

~ » decred/dcrctl --testnet --wallet sendrawtransaction 0100000001da72d6f35c9cc3aa248341885fa9526eecb3f45a4b9e98d9ed61a0bd086e952c0000000000ffffffff01924500000000000000001976a91412f1e1fb75d8ba8b0d7fa445d5125630aba3c69988ac000000000000000001204e00000000000000000000ffffffff914730440220597473ee2d1bb3581b7327c85f9245d395d339a5caf3ff65e7957d97e9927821022065b97b591c5fb9406a4b87b8424195690bdf152649dbb02c7a6b6636cfea4f2c0100475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae
0c639a378074d0fb710fd8c2aa81887e37b72aa61bd2b6f63b0b14f80c335569

dcrwallet log:

2023-07-25 13:39:42.376 [INF] WLLT: Inserting unconfirmed transaction 0c639a378074d0fb710fd8c2aa81887e37b72aa61bd2b6f63b0b14f80c335569

Additional info about this multisig address:

~ » decred/dcrctl --testnet --wallet getmultisigoutinfo 2c956e08bda061edd9989e4b5af4b3ec6e52a95f88418324aac39c5cf3d672da 0
{
  "address": "TcoAoGJUeo6dPojqgwKjWxUpPanncZEofoJ",
  "redeemscript": "5221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae",
  "m": 2,
  "n": 2,
  "pubkeys": [
    "030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a4145",
    "02dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af5"
  ],
  "txhash": "2c956e08bda061edd9989e4b5af4b3ec6e52a95f88418324aac39c5cf3d672da",
  "blockheight": 1176175,
  "blockhash": "b866a5bd3e13d42769f987de62604a4507f091331211823a783b994b9a2d13f0",
  "spent": true,
  "spentby": "0c639a378074d0fb710fd8c2aa81887e37b72aa61bd2b6f63b0b14f80c335569",
  "spentbyindex": 0,
  "amount": 0.0002
}

[norwnd]

While on this topic, from what I've gathered dcrwallet currently doesn't allow to construct a multisig transaction with specified Amount (and/or Fee) ? Any knowledge sharing on how DCR holders work with multisigs is welcome!

[norwnd]

I've built another multisig txn adding 1000 Atoms on top, and now it looks like I have an issue with txn itself:

0100000001da72d6f35c9cc3aa248341885fa9526eecb3f45a4b9e98d9ed61a0bd086e952c0000000000ffffffff01aa4100000000000000001976a914bb94feca4a7779cff6ce245775047208607c7fd288ac000000000000000001204e00000000000000000000ffffffff914730440220676fef287c1c80cab37dc90375bad2ff8eabdbd4187bc635b2d59848739da66202202c3058702138854042eecbe4ebafb15ab3fabef9fd10e9fb16de78f6dd1315c60100475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae

dcrdata error:

Error: -1: rejected transaction 6a8e80867a694f3a6a306195041a30e78de3f645444871ad8e870989403a3247: failed to validate input 6a8e80867a694f3a6a306195041a30e78de3f645444871ad8e870989403a3247:0 which references output 2c956e08bda061edd9989e4b5af4b3ec6e52a95f88418324aac39c5cf3d672da:0 - false stack entry at end of script execution (input script bytes 4730440220676fef287c1c80cab37dc90375bad2ff8eabdbd4187bc635b2d59848739da66202202c3058702138854042eecbe4ebafb15ab3fabef9fd10e9fb16de78f6dd1315c60100475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae, prev output script bytes a914abb90acd25884d6238b26a585caf123a7212a5e587)

which suggests I'm doing multisig wrong ... any hints on how to debug this ? in particular I'm not sure how to covert scriptSig (displayed on dcrdata when txn is decoded) to human-readable representation to make sense of it:

            "scriptSig": {
                "asm": "30440220676fef287c1c80cab37dc90375bad2ff8eabdbd4187bc635b2d59848739da66202202c3058702138854042eecbe4ebafb15ab3fabef9fd10e9fb16de78f6dd1315c601 0 5221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae",
                "hex": "4730440220676fef287c1c80cab37dc90375bad2ff8eabdbd4187bc635b2d59848739da66202202c3058702138854042eecbe4ebafb15ab3fabef9fd10e9fb16de78f6dd1315c60100475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae"
            }

[jrick]

Yes, it's a bug in our fee estimation for pay-to-script-hash transactions. The script and pubkeys are not being included in the estimated size.

The multisignature P2SH support was bolted on at the last minute to support the old stakepool (not VSP) code in which you created a ticket that is spendable by a vote by either your key or the stakepool's. Votes have no transaction fee, so this bug was never hit by that usecase.

(revocations do pay fee but i think some fudging was being done in that particular codepath that would overpay fee)

[jrick]

manually modifying the transaction returned by redeemmultisigout will not work. It adds signatures from that wallet before returning, but by modifying the transaction you have invalidated that signature.

The only way I think you are going to be able to spend this output now is to do the whole process manually with createrawtransaction. You will have to do the fee estimation yourself too.

[norwnd]

Got it,

which suggests I'm doing multisig wrong ... any hints on how to debug this ? in particular I'm not sure how to covert scriptSig (displayed on dcrdata when txn is decoded) to human-readable representation to make sense of it:

well, for future reference,

I've managed to compare my signing procedure to some multisig tests in dcrd repo and tracked the issue down to dcrwallet being locked when I'm trying to sign multisig transaction, yet returning "success" claiming it has signed successfully with "complete": true:

~ » decred/dcrctl --testnet --wallet redeemmultisigout 2c956e08bda061edd9989e4b5af4b3ec6e52a95f88418324aac39c5cf3d672da 0 0
{
  "hex": "0100000001da72d6f35c9cc3aa248341885fa9526eecb3f45a4b9e98d9ed61a0bd086e952c0000000000ffffffff01aa4100000000000000001976a914f76c08be6c43c094bfd3b823681152c7340cd5f588ac000000000000000001204e00000000000000000000ffffffff48475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae",
  "complete": true
}

possible improvement in that respect: decred/dcrd#3167.

[jrick]

I suspect we are hitting the error at

dcrwallet/wallet/wallet.go

Line 4915 in a38abe2

if errors.Is(err, txscript.ErrInvalidStackOperation) {

but it's not a stack underflow, so a signature error is never being recorded.

The actual error is likely txscript.ErrEvalFalse. Mind trying this patch?

diff /home/jrick/src/dcrwallet
commit - a38abe287ebe853c2eafec0921920507a77d4439
path + /home/jrick/src/dcrwallet
blob - c43d25def2c003a24682490a6a82c5f524940a22
file + internal/rpc/jsonrpc/methods.go
--- internal/rpc/jsonrpc/methods.go
+++ internal/rpc/jsonrpc/methods.go
@@ -4926,10 +4926,8 @@ func (s *Server) signRawTransaction(ctx context.Contex
 	// `complete' denotes that we successfully signed all outputs and that
 	// all scripts will run to completion. This is returned as part of the
 	// reply.
-	signErrs, err := w.SignTransaction(ctx, tx, hashType, inputs, keys, scripts)
-	if err != nil {
-		return nil, err
-	}
+	var signErr error
+	signErrs, signErr := w.SignTransaction(ctx, tx, hashType, inputs, keys, scripts)
 
 	var b strings.Builder
 	b.Grow(2 * tx.SerializeSize())
@@ -4952,7 +4950,7 @@ func (s *Server) signRawTransaction(ctx context.Contex
 
 	return types.SignRawTransactionResult{
 		Hex:      b.String(),
-		Complete: len(signErrors) == 0,
+		Complete: len(signErrors) == 0 && signErr == nil,
 		Errors:   signErrors,
 	}, nil
 }
blob - 44c7210aed2c563a1962e46564ec1bb48b17ef89
file + wallet/wallet.go
--- wallet/wallet.go
+++ wallet/wallet.go
@@ -4823,6 +4823,7 @@ func (w *Wallet) SignTransaction(ctx context.Context, 
 	err := walletdb.View(ctx, w.db, func(dbtx walletdb.ReadTx) error {
 		addrmgrNs := dbtx.ReadBucket(waddrmgrNamespaceKey)
 		txmgrNs := dbtx.ReadBucket(wtxmgrNamespaceKey)
+		var errEval error
 
 		for i, txIn := range tx.TxIn {
 			// For an SSGen tx, skip the first input as it is a stake base
@@ -4921,6 +4922,8 @@ func (w *Wallet) SignTransaction(ctx context.Context, 
 							multisigNotEnoughSigs = true
 						}
 					}
+				} else {
+					errEval = err
 				}
 				// Only report an error for the script engine in the event
 				// that it's not a multisignature underflow, indicating that
@@ -4934,10 +4937,10 @@ func (w *Wallet) SignTransaction(ctx context.Context, 
 				}
 			}
 		}
-		return nil
+		return errEval
 	})
 	if err != nil {
-		return nil, errors.E(op, err)
+		return signErrors, errors.E(op, err)
 	}
 	return signErrors, nil
 }

[jrick]

A couple other alternatives we could try:

• Remove the invalid signature
• Return the 'eval false' error up to the rpc caller directly instead of just setting complete=false. With the patch above, there isn't much indication of why it's false, and it doesn't particularly help you by removing the invalid signature (possibly replacing it with a good one).

[jrick]

broke the patch up above into a PR in case that is easier to test. Feel free to comment on it there.

[norwnd]

Thanks for looking into it! First, to clarify what complete response value actually means,

currently I believe it's: dcrwallet successfully provided ALL signatures it "owns" for this transaction, but transaction still might (or might not) need more signatures!

Return the 'eval false' error up to the rpc caller directly instead of just setting complete=false. With the patch above, there isn't much indication of why it's false, and it doesn't particularly help you by removing the invalid signature (possibly replacing it with a good one).

So, given that definition, it seems reasonable to treat the error we are after by setting complete=false, and not as a special error-edge-case (however overall I'd rather have a detailed error, just want to point out that if we go this route we probably want to return proper errors in other scenarios too; I'm not sure those errors can be meaningful to the API user given txscript package currently swallows them ... maybe that could be implemented separately later on if there is a need for this).

That's to say that errEval in your PR should probably not be something separate but rather just another item in signErrors (see also below, where I expand on this issue).

The actual error is likely txscript.ErrEvalFalse. Mind trying this patch?

I've tried your fix, it doesn't quite address my issue which is the following:

0. I'm trying to sign 2 of 2 multisig transaction (namely doing the first part: getting 1 of 2 signed transaction) with locked dcrwallet
1. sign.SignTxOutput func doesn't return error when we weren't successful fetching private key to sign with (because wallet is locked)
2. the actual error we get is txscript.ErrInvalidStackOperation and it exactly falls through into multisigNotEnoughSigs = true case, and it puts in redeemScript (prepping it with 47 in hex) without any signatures into txIn.SignatureScript:

475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae

while in successful scenario it'll put signature in there too:

47304402204cf54692b6e8c12c7494be9f446c485525538b2f1bdb092dbb5540ae157018fa02206db46aad8715023360b9d37f376e39496b855bce86c8db65ed75bcaddc9cd00801475221030e4db4d37cfa43553c645ad20ca79ae79eef966f41243628310e7624d33a41452102dc7aaeb575d3170760f3c719befd52805a0301b200a1e4efb700ebcc379a9af552ae

3. so it looks like if we want to catch this "ineffectual-signing" case we need to differentiate between those ^ (not quite sure how to do that in the best way though, parsing signature scripts in txIn.SignatureScript before/after ? I can implement something, if you give me some hints)

Remove the invalid signature

I think we should do that, seems simple to implement. But I think dcrwallet might try to sign multiple times, some of which might succeed and some not ... probably can just revert all signatures in that case. On a second thought, there is no point doing that. If I get and error or complete=false (under my definition of complete above) from this RPC endpoint I probably shouldn't be using it's output, and try to fix my issue (eg. unlock wallet) and try again or try to sign my original transaction with another wallet.

Perhaps another thing to consider is whether/how this update will affect GRPC API (I'm not sure #2274 results in consistent behavior between 2 APIs because GRPC will return error for this case while jsonrpc will "encode" this result into complete value). nah, that's looks OK.