Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCfddc6b49b69d: [validation] de-duplicate package transactions already in mempool

Summary

As node operators are free to set their mempool policies however they
please, it's possible for package transaction(s) to already be in the
mempool. We definitely don't want to reject the entire package in that
case (as that could be a censorship vector).

We should still return the successful result to the caller, so add
another result type to MempoolAcceptResult.

This concludes backport of core#22674
https://github.com/bitcoin/bitcoin/pull/22674/commits/e12fafda2dfbbdf63f125e5af797ecfaa6488f66
https://github.com/bitcoin/bitcoin/pull/22674/commits/046e8ff264be6b888c0f9a9d822e32aa74e19b78 (partial)

Depends on D12480

Notes:

because eCash does not have RBF, our constructor for MempoolAcceptResult (success case) would have the exact same signature as the new constructor (already-in-mempool). So in stead of adding a new constructor I made the existing one more generic to handle both cases. The rest of the codebase does not directly call this constructor, it always uses either MempoolAcceptResult::MempoolTx or MempoolAcceptResult::Success.
We do not need to handle the same-txid-diff-wtxid case, so the code in AcceptPackage is simplified for Bitcoin ABC.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

arcpatch-D12480

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 20940
Build 41536: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 41535: arc lint + arc unit

Event Timeline

PiRK created this revision.Nov 14 2022, 09:51

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 14 2022, 09:51

PiRK requested review of this revision.Nov 14 2022, 09:51

teamcity edited the summary of this revision. (Show Details)Nov 14 2022, 09:51

Harbormaster completed remote builds in B20894: Diff 36267.Nov 14 2022, 10:06

Fabien added a subscriber: Fabien.Nov 15 2022, 10:39

Fabien added inline comments.

src/validation.cpp
1093 ↗	(On Diff #36267)	See question in D12480, the same applies here

rebase

Harbormaster completed remote builds in B20933: Diff 36320.Nov 16 2022, 09:08

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

Tail of the build log:

[0m[0;32mwallet_importmulti.py                     | ✓ Passed  | 5 s
[0m[0;32mwallet_importprunedfunds.py               | ✓ Passed  | 2 s
[0m[0;32mwallet_importprunedfunds.py --descriptors | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool.py                         | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool_topup.py                   | ✓ Passed  | 4 s
[0m[0;32mwallet_keypool_topup.py --descriptors     | ✓ Passed  | 5 s
[0m[0;32mwallet_labels.py                          | ✓ Passed  | 2 s
[0m[0;32mwallet_labels.py --descriptors            | ✓ Passed  | 2 s
[0m[0;32mwallet_listreceivedby.py                  | ✓ Passed  | 8 s
[0m[0;32mwallet_listsinceblock.py                  | ✓ Passed  | 6 s
[0m[0;32mwallet_listsinceblock.py --descriptors    | ✓ Passed  | 8 s
[0m[0;32mwallet_listtransactions.py                | ✓ Passed  | 4 s
[0m[0;32mwallet_listtransactions.py --descriptors  | ✓ Passed  | 3 s
[0m[0;32mwallet_multiwallet.py                     | ✓ Passed  | 18 s
[0m[0;32mwallet_multiwallet.py --usecli            | ✓ Passed  | 16 s
[0m[0;32mwallet_reorgsrestore.py                   | ✓ Passed  | 5 s
[0m[0;32mwallet_resendwallettransactions.py        | ✓ Passed  | 14 s
[0m[0;32mwallet_send.py                            | ✓ Passed  | 8 s
[0m[0;32mwallet_startup.py                         | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_clone.py                       | ✓ Passed  | 4 s
[0m[0;32mwallet_txn_clone.py --mineblock           | ✓ Passed  | 4 s
[0m[0;32mwallet_txn_doublespend.py                 | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_doublespend.py --mineblock     | ✓ Passed  | 4 s
[0m[0;32mwallet_watchonly.py                       | ✓ Passed  | 2 s
[0m[0;32mwallet_watchonly.py --usecli              | ✓ Passed  | 2 s
[0m[1m
ALL                                       | ✓ Passed  | 1819 s (accumulated) 
[0mRuntime: 365 s

[33/481] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

OK
[34/481] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[196/481] Running seeder test suite
PASSED: seeder test suite
[218/481] Running pow test suite
PASSED: pow test suite
[371/481] bitcoin: testing txpackage_tests
FAILED: src/test/CMakeFiles/check-bitcoin-txpackage_tests 
cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-txpackage_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=txpackage_tests --logger=HRF,message:JUNIT,message,bitcoin-txpackage_tests.xml --catch_system_errors=no
Running 4 test cases...
Assertion failed: lock cs_main held in ../../src/validation.cpp:4516; locks held:
'cs_main' in ../../src/test/txpackage_tests.cpp:244 (in thread '')
Aborted (core dumped)
[452/481] Running avalanche test suite
PASSED: avalanche test suite
[458/481] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[478/481] Running secp256k1 test suite
PASSED: secp256k1 test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

rebase and add missing lock (now that D12480 no longer locks cs_main for the whole test)

Harbormaster completed remote builds in B20935: Diff 36322.Nov 16 2022, 09:30

Fabien accepted this revision.Nov 16 2022, 10:56

This revision is now accepted and ready to land.Nov 16 2022, 10:56

rebase

Harbormaster completed remote builds in B20940: Diff 36329.Nov 16 2022, 12:08

rebase

Harbormaster completed remote builds in B20944: Diff 36337.Nov 16 2022, 14:57

Closed by commit rABCfddc6b49b69d: [validation] de-duplicate package transactions already in mempool (authored by glozow <gloriajzhao@gmail.com>, committed by PiRK). · Explain WhyNov 17 2022, 09:52

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCfddc6b49b69d: [validation] de-duplicate package transactions already in mempool.