partial Merge #18224: Make AnalyzePSBT next role calculation simple, correct

1ef28b4f7cfba410fef524def1dac24bbc4086ca Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)

Pull request description:

  Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220

  Sjors documenting the issue:
  ```
  A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))

  {
    "inputs": [
      {
        "has_utxo": true,
        "is_final": false,
        "next": "finalizer"
      }
    ],
    "estimated_vsize": 141,
    "estimated_feerate": 1e-05,
    "fee": 1.41e-06,
    "next": "signer"
  }
  I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
  ```

  It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.

  Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.

ACKs for top commit:
  Sjors:
    ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca, much nicer. Don't forget to document the bug fix.
  achow101:
    ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca
  Empact:
    ACK 1ef28b4f7c

Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
This commit is contained in:
Samuel Dobson 2020-03-02 22:47:44 +13:00 committed by PastaPastaPasta
parent a5778e1db9
commit 68dfc06916
2 changed files with 18 additions and 20 deletions

View File

@ -322,9 +322,7 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
PSBTAnalysis result;
bool calc_fee = true;
bool all_final = true;
bool only_missing_sigs = true;
bool only_missing_final = false;
CAmount in_amt = 0;
result.inputs.resize(psbtx.tx->vin.size());
@ -333,6 +331,9 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
PSBTInput& input = psbtx.inputs[i];
PSBTInputAnalysis& input_analysis = result.inputs[i];
// We set next role here and ratchet backwards as required
input_analysis.next = PSBTRole::EXTRACTOR;
// Check for a UTXO
CTxOut utxo;
if (psbtx.GetInputUTXO(utxo, i)) {
@ -361,7 +362,6 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
// Check if it is final
if (!utxo.IsNull() && !PSBTInputSigned(input)) {
input_analysis.is_final = false;
all_final = false;
// Figure out what is missing
SignatureData outdata;
@ -377,11 +377,9 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
if (outdata.missing_pubkeys.empty() && outdata.missing_redeem_script.IsNull() && !outdata.missing_sigs.empty()) {
input_analysis.next = PSBTRole::SIGNER;
} else {
only_missing_sigs = false;
input_analysis.next = PSBTRole::UPDATER;
}
} else {
only_missing_final = true;
input_analysis.next = PSBTRole::FINALIZER;
}
} else if (!utxo.IsNull()){
@ -389,10 +387,14 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
}
}
if (all_final) {
only_missing_sigs = false;
result.next = PSBTRole::EXTRACTOR;
// Calculate next role for PSBT by grabbing "minumum" PSBTInput next role
result.next = PSBTRole::EXTRACTOR;
for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
PSBTInputAnalysis& input_analysis = result.inputs[i];
result.next = std::min(result.next, input_analysis.next);
}
assert(result.next > PSBTRole::CREATOR);
if (calc_fee) {
// Get the output amount
CAmount out_amt = std::accumulate(psbtx.tx->vout.begin(), psbtx.tx->vout.end(), CAmount(0),
@ -441,17 +443,6 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
result.estimated_feerate = feerate;
}
if (only_missing_sigs) {
result.next = PSBTRole::SIGNER;
} else if (only_missing_final) {
result.next = PSBTRole::FINALIZER;
} else if (all_final) {
result.next = PSBTRole::EXTRACTOR;
} else {
result.next = PSBTRole::UPDATER;
}
} else {
result.next = PSBTRole::UPDATER;
}
return result;

View File

@ -329,6 +329,13 @@ class PSBTTest(BitcoinTestFramework):
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 has invalid value')
self.log.info("PSBT with signed, but not finalized, inputs should have Finalizer as next")
analysis = self.nodes[0].analyzepsbt('cHNidP8BAHECAAAAAXfx5nfVEBDq0BbpUjqqfvNEWyX2dGCkx7RiW9E6DXPbAAAAAAD9////AlDDAAAAAAAAFgAUy/UxxZuzZswcmFnN/E9DGSiHLUsuGPUFAAAAABYAFLsH5o0R38wXx+X2cCosTMCZnQ4baAAAAAABACwCAAAAAAEA4fUFAAAAABl2qRTgSNoebYX9/i35W9ixgrFUmcLJbYisAAAAACICAv4uHbVQEQkVrUThtBx6BnGlZJcyVHMKOYaJoBpEjQJyRzBEAiBse8ynDCBq+3BBEvQUeCnbf6cz0yt3ylnBsgiV2+zFZwIgCzgYKpTB+BmwtRw4fx5spZjueH8hMuJSnXu/wU/z1scBAQMEAQAAACIGAv4uHbVQEQkVrUThtBx6BnGlZJcyVHMKOYaJoBpEjQJyGA8FaUNUAACAAQAAgAAAAIAAAAAAAAAAAAEBHwDh9QUAAAAAFgAU4EjaHm2F/f4t+VvYsYKxVJnCyW0AACICAv4IiIHn01IKyw4lEaIxhArVyFqGABpomkhcTjULKKv7GA8FaUNUAACAAQAAgAAAAIABAAAAAAAAAAA=')
# TODO update tx above.
# currently I can't figure out how to generate transaction, that would be valid and will show correct stage finalizer.
# currently it fails at `verify pubkey` for signer
# assert_equal(analysis['next'], 'finalizer')
analysis = self.nodes[0].analyzepsbt('cHNidP8BAHECAAAAAbmNr2X0Nc+2AtaXYjPu3kIFc3Cmj1aYy5WQs5yaUfRvAAAAAAD/////AgCAgWrj0AcAFgAUKNw0x8HRctAgmvoevm4u1SbN7XL87QKVAAAAABYAFPck4gF7iL4NL4wtfRAKgQbghiTUAAAAAAABABQCAAAAAAGghgEAAAAAAAFRAAAAAAEBHwDyBSoBAAAAFgAUlQO3F/Y8ejrjUcQ4E4Ai8Uw1OvYAAAA=')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Output amount invalid')