Skip to content

Conversation

@theStack
Copy link
Contributor

The functional test mempool_packages.py starts one node with default ancestor/descendant limit settings and one with a custom, reduced ancestor limit (currently -limitancestorcount=5). The effect of the latter had not been tested yet though. This is approached in this PR by checking on the expected mempool contents of node1 after the node0 ancestor tests are done, via the following three conditions:

  • the # of txs in the node1 mempool is equal to the the limit
  • all txs in node1 mempool are a subset of txs in node0 mempool
  • the node1 mempool txs match the start of the constructed tx-chain

Note that this still doesn't fully check the expected mempool of node1 (e.g. that it isn't influenced by prioritisetransaction RPC on node0), hence I add another TODO. In the future it would make sense to also set a custom descendant limit when the second TODO about checking node1's mempool is approached:

# TODO: check that node1's mempool is as expected

@fanquake fanquake added the Tests label Nov 10, 2019
@maflcko
Copy link
Member

maflcko commented Nov 11, 2019

LGTM, sorry for nitpicking. Feel free to ignore those

To test the custom ancestor limit on node1 (passed by the argument
-limitancestorcount), we check for three conditions:
    -> the # of txs in the node1 mempool is equal to the the limit
    -> all txs in node1 mempool are a subset of txs in node0 mempool
    -> the node1 mempool txs match the start of the constructed tx-chain
@theStack theStack force-pushed the 20191110-test-check_custom_ancestor_limit_in_mempool-packages branch from fce1f72 to 4999781 Compare November 11, 2019 21:38
@theStack
Copy link
Contributor Author

@MarcoFalke: Thanks for reviewing, always glad to get feedback, even though its only nits. (I have to admit I wasn't even aware that the parantheses can be omitted for Python asserts... guess I assumed its just a function call).

@maflcko
Copy link
Member

maflcko commented Nov 12, 2019

ACK 4999781 👲

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 49997813a4db388b2810e5e27ef771e8aa6a1f03 👲
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj3dwwAyzXxmPEnvF4kSWp0hSjOwaT8zmwr0PzXqJ+x9ZuwIFqEwMkxq8gLP9GE
OL+N16kkFdXU+NVNKR+yBVO6SxGGIjWc1Lgf7cgkqsT+AQyqJ6ZUyxVQAYWX6aE/
2yRbLh91jM6g9xPrS4aRQmzBRuTXbXjR8vaazytmRDRxYJvbyh2hVUbJLk3+9N/s
+sUJ9bUQLT4nOa4/xCRPUaP524j5/2iGIIY3zjiCS5cB0fgu9R1ECeouNZKYCRGk
PdCVDmnRe80cf6gQ4oMLVUXffTJGkDZ3v1ioiZyypNCXD2LFWk1/F/Rx2cO1HFTx
DDizqNdRZYwJvN8PdHH+RXie9qtjn6CPZDH95GX7VQMRL3MzR79jGgNKcDaRnm1o
4/HyYbRRcODodAAG1bdWeSM6FA/3eCH5hQfCW7YXakTbOEAWX62T1SrunQ7AseJ4
jXu4jRjjJ8kud8dAqHKK1ahORiamnRvbEyfFjB0J8lnbgInhbPKXRtqvVfkdSfr7
539CNBMb
=1woH
-----END PGP SIGNATURE-----

Timestamp of file with hash 7d1507105d97dc06921b89a9abfadd83a2af4691c89687f45ab2592868d52a25 -

maflcko pushed a commit that referenced this pull request Nov 12, 2019
4999781 test: check custom ancestor limit in mempool_packages.py (Sebastian Falbesoner)

Pull request description:

  The functional test `mempool_packages.py` starts one node with default ancestor/descendant limit settings and one with a custom, reduced ancestor limit (currently `-limitancestorcount=5`). The effect of the latter had not been tested yet though. This is approached in this PR by checking on the expected mempool contents of node1 after the node0 ancestor tests are done, via the following three conditions:
  - the # of txs in the node1 mempool is equal to the the limit
  - all txs in node1 mempool are a subset of txs in node0 mempool
  - the node1 mempool txs match the start of the constructed tx-chain

  Note that this still doesn't *fully* check the expected mempool of node1 (e.g. that it isn't influenced by `prioritisetransaction` RPC on node0), hence I add another TODO. In the future it would make sense to also set a custom descendant limit when the second TODO about checking node1's mempool is approached: https://github.com/bitcoin/bitcoin/blob/89e93135aedf984f7a98771f047e2beb6cdbdb8e/test/functional/mempool_packages.py#L228

ACKs for top commit:
  MarcoFalke:
    ACK 4999781 👲

Tree-SHA512: d3a1d19fb49731238ad08ee7c02e2fa81a227e3b4ef3340d68598de42ddb62be9161134f6b8e08fa76b8c9faa02fecfa01111159642e20e9f358292a757b7608
@maflcko maflcko merged commit 4999781 into bitcoin:master Nov 12, 2019
maflcko pushed a commit that referenced this pull request Feb 27, 2020
b902bd6 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to #17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
  1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
  2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~

  ~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
  - the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
  - all txs in node1 mempool are a subset of txs in node0 mempool
  - part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
  - the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool

ACKs for top commit:
  JeremyRubin:
    Excellent. utACK b902bd6

Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 28, 2020
…ackages.py

b902bd6 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to bitcoin#17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
  1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
  2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~

  ~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
  - the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
  - all txs in node1 mempool are a subset of txs in node0 mempool
  - part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
  - the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool

ACKs for top commit:
  JeremyRubin:
    Excellent. utACK b902bd6

Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…ackages.py

b902bd6 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to bitcoin#17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
  1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
  2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~

  ~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
  - the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
  - all txs in node1 mempool are a subset of txs in node0 mempool
  - part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
  - the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool

ACKs for top commit:
  JeremyRubin:
    Excellent. utACK b902bd6

Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
@theStack theStack deleted the 20191110-test-check_custom_ancestor_limit_in_mempool-packages branch December 1, 2020 09:57
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants