Update PSBT test #627
Update PSBT test #627
Conversation
* Finalization for multisig was counting `OP_0` as real signature, so fix. * In v0.17 of bitcoin core, `importmulti` does not recognize witness script, so ignore for now.
* Rename `GetSigPushes` to more Generic `GetPushItems` * throw error when tried to finalize PSBT Input with unknown type of script.
|
FWIW, segwit support for importmulti is merged in this pr |
* Run ScriptVerify before setting final_script* * Capture Errors in `TryFinalize`
joemphilips
force-pushed
the
psbt.walletcreatefundedpsbt
branch
from
f6d3b8d
to
8b84caf
Compare
joemphilips
force-pushed
the
psbt.walletcreatefundedpsbt
branch
from
a5bbb0f
to
e3b93af
Compare
… and coins separately
|
Thanks, reviewing. Next time can you make several PRs instead of one big? it is faster to review. |
| @@ -311,10 +311,19 @@ private void DeconstructTxIn(TxIn txin) | |||
|
|
|||
| internal void AddCoin(ICoin coin, TxIn txin = null) | |||
| { | |||
| if (coin is ScriptCoin && !IsFinalized() && txin != null) | |||
| var scriptCoin = coin as ScriptCoin; | |||
| if (scriptCoin != null && !IsFinalized() && txin != null) | |||
There was a problem hiding this comment.
You can leave it like this, but protip to simplify:
- var scriptCoin = coin as ScriptCoin;
- if (scriptCoin != null && !IsFinalized() && txin != null)
+ if (coin is ScriptCoin scriptCoin && !IsFinalized() && txin != null)| @@ -1404,8 +1406,8 @@ public void ShouldWalletProcessPSBTAndExtractMempoolAcceptableTX() | |||
| result = client.WalletProcessPSBT(psbtUnFinalized, false, type, bip32derivs: true); | |||
| Assert.False(result.Complete); | |||
| Assert.False(result.PSBT.CanExtractTX()); | |||
| result.PSBT.TryFinalize(out bool isFinalized2); | |||
| Assert.False(isFinalized2); | |||
| result.PSBT.TryFinalize(out var errors2); | |||
There was a problem hiding this comment.
I think you should still Assert.False this.
There was a problem hiding this comment.
I have changed the TryFinalize API to take out InvalidOperationException[] instead of out bool so that the user can see why (and which input) is failing to finalize (breaking change). So Assert.NotEmpty below is equal to previous Assert.False
Sorry that I have put too many changes in one PR, I will split.
| // with 2 difference. | ||
| // 1. one user (David) do not use bitcoin core (only NBitcoin) | ||
| // 2. 4-of-4 instead of 2-of-3 | ||
| // 3. In version 0.17, `importmulti` can not handle witness script so only p2sh are considered here. TODO: fix |
There was a problem hiding this comment.
not fixed in 0.17.1 ? :(
There was a problem hiding this comment.
I have tried but its not fixed :(
| @@ -1404,8 +1406,8 @@ public void ShouldWalletProcessPSBTAndExtractMempoolAcceptableTX() | |||
| result = client.WalletProcessPSBT(psbtUnFinalized, false, type, bip32derivs: true); | |||
| Assert.False(result.Complete); | |||
| Assert.False(result.PSBT.CanExtractTX()); | |||
| result.PSBT.TryFinalize(out bool isFinalized2); | |||
| Assert.False(isFinalized2); | |||
| result.PSBT.TryFinalize(out var errors2); | |||
There was a problem hiding this comment.
I have changed the TryFinalize API to take out InvalidOperationException[] instead of out bool so that the user can see why (and which input) is failing to finalize (breaking change). So Assert.NotEmpty below is equal to previous Assert.False
Sorry that I have put too many changes in one PR, I will split.
| // with 2 difference. | ||
| // 1. one user (David) do not use bitcoin core (only NBitcoin) | ||
| // 2. 4-of-4 instead of 2-of-3 | ||
| // 3. In version 0.17, `importmulti` can not handle witness script so only p2sh are considered here. TODO: fix |
There was a problem hiding this comment.
I have tried but its not fixed :(
walletcreatefundedpsbtrpcgetaddressinforpc (by biproduct)