Differential D7809

[backport#16821] Fix bug where duplicate PSBT keys are accepted
ClosedPublic
Actions

Authored by majcosta on Oct 7 2020, 20:20.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC5b58baa0d841: [backport#16821] Fix bug where duplicate PSBT keys are accepted

Summary

9743432034586385cfef87df4b377c255ed0cba8 Fix bug where duplicate PSBT keys are accepted (John L. Jegutanis)

Pull request description:

As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.

For example, the following PSBT, included in the test vectors,
contains a duplicate field:

```
// magic
70736274ff

// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00

// no inputs

// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```

ACKs for top commit:

achow101:
  ACK 9743432034586385cfef87df4b377c255ed0cba8
instagibbs:
  code review ACK https://github.com/bitcoin/bitcoin/pull/16821/commits/9743432034586385cfef87df4b377c255ed0cba8

Backport of Core PR16821

Test Plan

ninja all check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

majcosta created this revision.Oct 7 2020, 20:20

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2020, 20:20

majcosta requested review of this revision.Oct 7 2020, 20:20

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Harbormaster completed remote builds in B13250: Diff 24363.Oct 7 2020, 20:37

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Snippet of first build failure:

[0m[1;30mwallet_hd.py                                     | ○ Skipped | 0 s
[0m[1;30mwallet_import_rescan.py                          | ○ Skipped | 0 s
[0m[1;30mwallet_import_with_label.py                      | ○ Skipped | 0 s
[0m[1;30mwallet_importmulti.py                            | ○ Skipped | 0 s
[0m[1;30mwallet_importprunedfunds.py                      | ○ Skipped | 0 s
[0m[1;30mwallet_keypool.py                                | ○ Skipped | 0 s
[0m[1;30mwallet_keypool_topup.py                          | ○ Skipped | 0 s
[0m[1;30mwallet_labels.py                                 | ○ Skipped | 0 s
[0m[1;30mwallet_listreceivedby.py                         | ○ Skipped | 0 s
[0m[1;30mwallet_listsinceblock.py                         | ○ Skipped | 0 s
[0m[1;30mwallet_listtransactions.py                       | ○ Skipped | 0 s
[0m[1;30mwallet_multiwallet.py                            | ○ Skipped | 0 s
[0m[1;30mwallet_multiwallet.py --usecli                   | ○ Skipped | 0 s
[0m[1;30mwallet_reorgsrestore.py                          | ○ Skipped | 0 s
[0m[1;30mwallet_resendwallettransactions.py               | ○ Skipped | 0 s
[0m[1;30mwallet_txn_clone.py                              | ○ Skipped | 0 s
[0m[1;30mwallet_txn_clone.py --mineblock                  | ○ Skipped | 0 s
[0m[1;30mwallet_txn_doublespend.py                        | ○ Skipped | 0 s
[0m[1;30mwallet_txn_doublespend.py --mineblock            | ○ Skipped | 0 s
[0m[1;30mwallet_watchonly.py                              | ○ Skipped | 0 s
[0m[1;30mwallet_watchonly.py --usecli                     | ○ Skipped | 0 s
[0m[1;30mwallet_zapwallettxes.py                          | ○ Skipped | 0 s
[0m[0;31mp2p_timeouts.py                                  | ✖ Failed  | 8 s
[0m[0;31m[1m
ALL                                              | ✖ Failed  | 379 s (accumulated) 
[0m[0mRuntime: 81 s

[131/373] Running avalanche test suite
PASSED: avalanche test suite
[143/373] Running seeder test suite
PASSED: seeder test suite
[151/373] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

OK
[152/373] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[159/373] Running pow test suite
PASSED: pow test suite
[166/373] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[168/373] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:541:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
[372/373] Running bitcoin test suite
PASSED: bitcoin test suite
FAILED: test/CMakeFiles/check-functional 
cd /work/abc-ci-builds/build-without-wallet/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /usr/bin/python3.7 ./functional/test_runner.py "--testsuitename=Bitcoin ABC functional tests" --junitoutput=/work/abc-ci-builds/build-without-wallet/test/junit/functional_tests.xml
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Each failure log is accessible here:
Bitcoin ABC functional tests: p2p_timeouts.py

deadalnix accepted this revision.Oct 7 2020, 22:35

This revision is now accepted and ready to land.Oct 7 2020, 22:35

Closed by commit rABC5b58baa0d841: [backport#16821] Fix bug where duplicate PSBT keys are accepted (authored by fanquake <fanquake@gmail.com>, committed by majcosta). · Explain WhyOct 7 2020, 23:08

This revision was automatically updated to reflect the committed changes.

majcosta added a commit: rABC5b58baa0d841: [backport#16821] Fix bug where duplicate PSBT keys are accepted.