[mempool] check ancestor/descendant limits for packages
ClosedPublic
Actions

Authored by PiRK on Oct 6 2022, 09:33.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC97b9dbb0e79c: [mempool] check ancestor/descendant limits for packages

Summary

[refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits

This does not change existing behavior.
The ancestor/descendant limits are inclusive of the entries themselves,
but CalculateAncestorsAndCheckLimits() does not need access to them.

https://github.com/bitcoin/bitcoin/pull/21800/commits/f551841d3ec080a2d7a7988c7b35088dff6c5830

[mempool] check ancestor/descendant limits for packages

When calculating ancestor/descendant counts for transactions in the
package, as a heuristic, count every transaction in the package as an
ancestor and descendant of every other transaction in the package.

This may overestimate, but will not underestimate, the
ancestor/descendant counts. This shortcut still produces an accurate
count for packages of 1 parent + 1 child.

https://github.com/bitcoin/bitcoin/pull/21800/commits/c6e016aa139c8363e9b38bbc1ba0dca55700b8a7

[policy] ancestor/descendant limits for packages

https://github.com/bitcoin/bitcoin/pull/21800/commits/3cd663a5d33aa7ef87994e452bced7f192d021a0

[test] helper function to increase transaction weight

Use our existing pad_tx function.

https://github.com/bitcoin/bitcoin/pull/21800/commits/313c09f7b7beddfdb74c284720d209c81dfdb94f

[test] parameterizable fee for make_chain and create_child_with_parents

https://github.com/bitcoin/bitcoin/pull/21800/commits/2b6b26e57c24d2f0abd442c1c33098e3121572ce

[test] mempool package ancestor/descendant limits

https://github.com/bitcoin/bitcoin/pull/21800/commits/accf3d5868460b4b14ab607fd66ac985b086fbb3

Notes;

tx.rehash() replaced with tx.get_id()
used helper functions ToHex and FromHex. tx_from_hex is not available due to missing backports.
our limit for number of transactions in a package is 50. This leads to some minor logical differences in the tests, because where Core uses 24 transactions (an even number) we use 49 (an odd number). This leads to some asymetric chain transactions branches where the source material has symetric branches, and vice-versa.

This concludes backport of core#21800

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr21800

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 20388
Build 40449: Build Diff	build-debug · build-without-wallet · build-clang · lint-circular-dependencies · build-diff · build-clang-tidy
Build 40448: arc lint + arc unit

Event Timeline

PiRK created this revision.Oct 6 2022, 09:33

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 6 2022, 09:33

PiRK requested review of this revision.Oct 6 2022, 09:33

PiRK edited the summary of this revision. (Show Details)Oct 6 2022, 09:33

PiRK edited the summary of this revision. (Show Details)

PiRK edited the summary of this revision. (Show Details)Oct 6 2022, 09:36

teamcity edited the summary of this revision. (Show Details)Oct 6 2022, 09:38

Harbormaster completed remote builds in B20378: Diff 35468.Oct 6 2022, 09:48

Mostly a sum of nits

src/txmempool.h
752 ↗	(On Diff #35468)	update the doc with a more relevant example (50)
759 ↗	(On Diff #35468)
763 ↗	(On Diff #35468)
test/functional/mempool_package_limits.py
83 ↗	(On Diff #35468)	nit: res["allowed"] is True
87 ↗	(On Diff #35468)
99 ↗	(On Diff #35468)
99 ↗	(On Diff #35468)	See nits below, but generally these comments are useless as they add no value, better use named args for the same doc and a single place to maintain
101 ↗	(On Diff #35468)
186 ↗	(On Diff #35468)	dito
248 ↗	(On Diff #35468)	dito
310 ↗	(On Diff #35468)	dito
367 ↗	(On Diff #35468)	dito
436 ↗	(On Diff #35468)	dito
522 ↗	(On Diff #35468)	dito
test/functional/test_framework/wallet.py
169 ↗	(On Diff #35468)	why not importing only deepcopy like the source material ?
169 ↗	(On Diff #35468)	The PR's assertion that the required size is >= tx size can be preserved
170 ↗	(On Diff #35468)

This revision now requires changes to proceed.Oct 6 2022, 14:41

PiRK marked 2 inline comments as done.Oct 6 2022, 16:33

PiRK added inline comments.

test/functional/test_framework/wallet.py
169 ↗	(On Diff #35468)	why not importing only deepcopy like the source material ? OK, I guess it is better to only make the needed function accessible. If had used`from copy import copy`, then I would have preferred prefer the `import copy; obj2 = copy.copy(obj)` solution to avoid any ambiguity and risk of shadowing a previous import of the module. That's why I'm used to importing the entire namespace for this one. Note that in general for python it does not make a difference in terms of performance (both solutions will cause the entire module to be parsed / executed), it only affects which members of `copy` are accessible in the namespace.
169 ↗	(On Diff #35468)	OK. Note that in the source material the comparison is backwards ( thing1 >= thing2), so I will use `assert_greater_than_or_equal(tx_heavy.size(), target_size)` instead of `assert_greater_than_or_equal(target_size, tx_heavy.size())`