Page MenuHomePhabricator
Differential D12480

[validation] package accept + mempool submission, require packages to be child-with-unconfirmed-parents
ClosedPublic
Actions

Authored by PiRK on Nov 14 2022, 08:42.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC5b41420a65f2: [validation] package accept + mempool submission, require packages to be child…
Summary

[policy] require submitted packages to be child-with-unconfirmed-parents

https://github.com/bitcoin/bitcoin/pull/22674/commits/144a29099a865ac1dc3e5291d9529fbcca9c83a4

[validation] full package accept + mempool submission

https://github.com/bitcoin/bitcoin/pull/22674/commits/be3ff151a1f9665720cdf70d072b098a2f9726a9

[packages] add sanity checks for package vs mempool limits

https://github.com/bitcoin/bitcoin/pull/22674/commits/8310d942e046c5a9b6bd90afdcd3af68dd91e081

[unit test] package submission

https://github.com/bitcoin/bitcoin/pull/22674/commits/046e8ff264be6b888c0f9a9d822e32aa74e19b78

Note: the last test for Already-in-mempool transactions is skipped and will be with in the next commit.

This is a partial backport of core#22674

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Nov 14 2022, 08:42
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 14 2022, 08:42
PiRK requested review of this revision.Nov 14 2022, 08:42
teamcity edited the summary of this revision. (Show Details)Nov 14 2022, 08:43
PiRK edited the summary of this revision. (Show Details)Nov 14 2022, 08:43
Harbormaster completed remote builds in B20893: Diff 36266.Nov 14 2022, 09:25
PiRK added a child revision: D12481: [validation] de-duplicate package transactions already in mempool.Nov 14 2022, 09:51
Fabien requested changes to this revision.Nov 15 2022, 10:24
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/validation.cpp
358 ↗(On Diff #36266)
1048 ↗(On Diff #36266)

Actually if you submit a package with all parents being confirmed, it will not return this error despite its name. I also think it will be accepted with no error. Is that the expected behavior ?

1120 ↗(On Diff #36266)

This looks misplaced (but same in PR)

This revision now requires changes to proceed.Nov 15 2022, 10:24
Fabien mentioned this in D12481: [validation] de-duplicate package transactions already in mempool.Nov 15 2022, 10:39
Fabien added inline comments.Nov 15 2022, 16:19
src/validation.cpp
860 ↗(On Diff #36266)
PiRK marked 3 inline comments as done.Nov 16 2022, 07:33
PiRK updated this revision to Diff 36319.Nov 16 2022, 08:31
PiRK edited the summary of this revision. (Show Details)
PiRK added inline comments.Nov 16 2022, 08:43
src/validation.cpp
1048 ↗(On Diff #36266)

For now the code at this stage is not able to detect when a transaction in the package is already confirmed. It will fail later when it actually tries to submit transactions to the mempool.

Harbormaster completed remote builds in B20932: Diff 36319.Nov 16 2022, 08:50
teamcity added a comment.Nov 16 2022, 08:50
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

wallet_importprunedfunds.py               | ✓ Passed  | 3 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 3 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 3 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 8 s
wallet_labels.py                          | ✓ Passed  | 5 s
wallet_labels.py --descriptors            | ✓ Passed  | 5 s
wallet_listreceivedby.py                  | ✓ Passed  | 9 s
wallet_listsinceblock.py                  | ✓ Passed  | 7 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 10 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 3 s
wallet_multiwallet.py                     | ✓ Passed  | 21 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 16 s
wallet_reorgsrestore.py                   | ✓ Passed  | 4 s
wallet_resendwallettransactions.py        | ✓ Passed  | 3 s
wallet_send.py                            | ✓ Passed  | 9 s
wallet_startup.py                         | ✓ Passed  | 3 s
wallet_txn_clone.py                       | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 4 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 2 s

ALL                                       | ✓ Passed  | 1855 s (accumulated) 
Runtime: 372 s

[172/481] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.006s

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

OK
[210/481] Running seeder test suite
PASSED: seeder test suite
[368/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:4475; locks held:
'cs_main' in ../../src/test/txpackage_tests.cpp:244 (in thread '')
Aborted (core dumped)
[439/481] Running secp256k1 test suite
PASSED: secp256k1 test suite
[451/481] Running avalanche test suite
PASSED: avalanche test suite
[453/481] Running pow test suite
PASSED: pow test suite
[466/481] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[478/481] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1
PiRK updated this revision to Diff 36321.Nov 16 2022, 09:09

don't lock cs_main when calling CreateAndProcessBlock because ProcessNewBlock asserts that it is not held

Harbormaster completed remote builds in B20934: Diff 36321.Nov 16 2022, 09:31
PiRK updated this revision to Diff 36328.Nov 16 2022, 11:31

rebase onto master

Harbormaster completed remote builds in B20939: Diff 36328.Nov 16 2022, 11:50
Fabien requested changes to this revision.Nov 16 2022, 14:02
Fabien added inline comments.
src/test/txpackage_tests.cpp
400 ↗(On Diff #36328)

You need to check the tx error is the expected one.
You expect "bad-txns-inputs-missingorspent" or "txn-already-known" but here you'll get "bad-txns-premature-spend-of-coinbase" so it's not testing what you imagine

This revision now requires changes to proceed.Nov 16 2022, 14:02
Fabien added inline comments.Nov 16 2022, 14:04
src/test/txpackage_tests.cpp
401 ↗(On Diff #36328)
PiRK updated this revision to Diff 36336.Nov 16 2022, 14:25

use the first coinbase as confirmed parent to get trigger "txn-already-known" instead of "bad-txns-premature-spend-of-coinbase", adjust the pool size accordingly

Harbormaster completed remote builds in B20943: Diff 36336.Nov 16 2022, 14:43
Fabien accepted this revision.Nov 16 2022, 16:56
Fabien added inline comments.
src/test/txpackage_tests.cpp
381 ↗(On Diff #36336)

Please add a comment explaining why you need it to make the code easier to follow

This revision is now accepted and ready to land.Nov 16 2022, 16:56
PiRK updated this revision to Diff 36361.Nov 17 2022, 07:20

add a comment for expected_pool_size -= 1;, and don't rebuild txns that already exist from a previous test.

Harbormaster completed remote builds in B20965: Diff 36361.Nov 17 2022, 07:32
Closed by commit rABC5b41420a65f2: [validation] package accept + mempool submission, require packages to be child… (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: rABC5b41420a65f2: [validation] package accept + mempool submission, require packages to be child….

Revision Contents
Changeset List

PathSize
src/
policy/
12 lines
test/
155 lines
5 lines
252 lines

Diff 36362

View Options

src/policy/mempool.h

Loading...
View Options

src/test/txpackage_tests.cpp

Loading...
View Options

src/validation.h

Loading...
View Options

src/validation.cpp

Loading...