Page MenuHomePhabricator

[mempool] check ancestor/descendant limits for packages
ClosedPublic

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

Event Timeline

PiRK requested review of this revision.Oct 6 2022, 09:33
PiRK edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Oct 6 2022, 14:41
Fabien added a subscriber: Fabien.

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)

Macro likestamp:

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())

PiRK marked an inline comment as done.

review

PiRK marked 6 inline comments as done.Oct 6 2022, 16:39
This revision is now accepted and ready to land.Oct 7 2022, 10:25