AcceptMultipleTransactions: Fix workspace not being set as client

instagibbs commented at 2:41 pm on March 26, 2024: member

Bug causes an Assume() failure due to the expectation that the individual result should be invalid when done over submitpackage via rpc.

Bug introduced by #28950 , and I discovered it rebasing #28984 since it’s easier to hit in that test scenario.

Tests in place were only checking AcceptSingleTransaction-level checks due to package evaluation only triggering when minfee is too high for the parent transaction.

Added test along with fix, moving the fill_mempool utility into a common area for re-use.

DrahtBot commented at 2:41 pm on March 26, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, theStack, ismaelsadeeq
Stale ACK	murchandamus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#28970 (p2p: opportunistically accept 1-parent-1-child packages by glozow)
#28531 (improve MallocUsage() accuracy by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/test_framework/util.py:504 in aaa5125f1c outdated

495@@ -496,6 +496,48 @@ def check_node_connections(*, node, num_in, num_out):
496     assert_equal(info["connections_in"], num_in)
497     assert_equal(info["connections_out"], num_out)
498 
499+# Allows simpler testing of scenarios with minfee != minrelay
500+def fill_mempool(test_framework, node, miniwallet):
501+        """Fill mempool until eviction."""

glozow commented at 2:51 pm on March 26, 2024:

is there an extra indent for this whole block? Also, might want to add some other things to this comment like

-maxmempool needs to be very small (i.e. 5) for this to work
if there are multiple nodes, note this doesn’t run sync_mempools
assumes that -minrelaytxfee is 1sat/vB

instagibbs commented at 12:56 pm on March 27, 2024:

added some comments

glozow commented at 2:37 pm on March 28, 2024:

I meant to add docs in the docstring comment :sweat_smile: also:

requires -datacarriersize=100000
I think you mean “It will not ensure mempools become synced” ?

instagibbs commented at 8:26 am on April 4, 2024:

oops yeah, done

glozow added the label Validation on Mar 26, 2024

in test/functional/test_framework/util.py:499 in aaa5125f1c outdated

495@@ -496,6 +496,48 @@ def check_node_connections(*, node, num_in, num_out):
496     assert_equal(info["connections_in"], num_in)
497     assert_equal(info["connections_out"], num_out)
498 
499+# Allows simpler testing of scenarios with minfee != minrelay
500+def fill_mempool(test_framework, node, miniwallet):

glozow commented at 10:48 am on March 27, 2024:

IIUC util is for more low-level stuff, but don’t have better ideas other than throwing it on test_framework.py. Maybe @maflcko or @theStack would have an opinion on where we should put this?

instagibbs commented at 12:46 pm on March 27, 2024:

it has mine_large_block et al which seem quite similar imo. I’ll move it if there’s something obviously better?

theStack commented at 6:01 pm on March 27, 2024:

IIUC util is for more low-level stuff, but don’t have better ideas other than throwing it on test_framework.py. Maybe @maflcko or @theStack would have an opinion on where we should put this?

No strong opinion or better idea from my side either, util.py seems fine IMHO.

in test/functional/rpc_packages.py:439 in 5ae4dce79d outdated

430-        assert "error" not in pkg_result["tx-results"][chained_txns[0]["wtxid"]]
431+        pkg_result = node.submitpackage(chained_burn_hex, maxfeerate=minrate_btc_kvb_burn, maxburnamount=chained_txns_burn[1]["new_utxo"]["value"])
432+        assert "error" not in pkg_result["tx-results"][chained_txns_burn[0]["wtxid"]]
433         assert_equal(pkg_result["tx-results"][tx.getwtxid()]["error"], "scriptpubkey")
434-        assert_equal(node.getrawmempool(), [chained_txns[0]["txid"]])
435+        assert_equal(node.getrawmempool(), [chained_txns_burn[0]["txid"]])

glozow commented at 11:00 am on March 27, 2024:

unrelated renames?

instagibbs commented at 12:49 pm on March 27, 2024:

was confusing to me to re-use the names for two distinct transaction chains (yeah I wrote the original test, so what?)

in src/validation.cpp:1371 in 5ae4dce79d outdated

1365@@ -1366,6 +1366,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1366         // Individual modified feerate exceeded caller-defined max; abort
1367         // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust.
1368         if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) {
1369+            // Need to set failure here both individually and at package level
1370+            ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", "");
1371             package_state.Invalid(PackageValidationResult::PCKG_TX, "max feerate exceeded");

glozow commented at 11:02 am on March 27, 2024:

Should this be “transaction failed” since it’s an individual error?

instagibbs commented at 12:55 pm on March 27, 2024:

changed

glozow commented at 11:08 am on March 27, 2024: member

Concept ACK, verified that on master the test fails since the individual result wasn’t populated and is “Valid”

instagibbs force-pushed on Mar 27, 2024

instagibbs commented at 1:31 pm on March 27, 2024: member

Added some fuzz coverage which would have likely caught this

edit: yep, 3 minutes: LC1hZGRuCS0Ab2VtZHL1iQHY2NjY2Am//wAAAAD/AACAAAAAAAAAAAAAAAAAAAAAFAAAAAAAAAAA//8AAABB44MP3NL5/4zzEoz/dpB2dg==

in test/functional/rpc_packages.py:39 in 4f8407cefa outdated

31@@ -31,6 +32,11 @@ def set_test_params(self):
32         self.setup_clean_chain = True
33         # whitelist peers to speed up tx relay / mempool sync
34         self.noban_tx_relay = True
35+        # Needed for fill_mempool
36+        self.extra_args = [[
37+            "-datacarriersize=100000",
38+            "-maxmempool=5",
39+        ]]

glozow commented at 4:25 pm on April 3, 2024:

nit 4f8407cefa6ddb47b166b2c360d03778c0357a00: would prefer to do a restart in the subtest, to minimize the amount of tests that we do on a nondefault settings

instagibbs commented at 8:26 am on April 4, 2024:

done

glozow commented at 6:43 pm on April 3, 2024: member

Sorry for the nits, still need to run the fuzzer but otherwise lgtm

instagibbs force-pushed on Apr 4, 2024

glozow commented at 8:58 am on April 4, 2024: member

ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4

Verified the fuzz test catches this problem, seems to run fine with the changes. Thanks for accepting the suggestions.

DrahtBot added the label CI failed on Apr 4, 2024

in test/functional/test_framework/util.py:523 in cc770e4b36 outdated

521+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
522+        # And 2 more for the package cpfp test
523+        test_framework.generate(miniwallet, 1 + (num_of_batches * tx_batch_size))
524+
525+        # Mine 99 blocks so that the UTXOs are allowed to be spent
526+        test_framework.generate(node, 99)

murchandamus commented at 6:08 pm on April 4, 2024:

In “Move fill_mempool to util function” (cc770e4b36e6f04cbd588ad3e7a19f122c21d38d):

Nit: If a commit is described as a move, I would expect no changes in the code beyond what’s necessary to update the callsites. While these changes are minor and obviously do not entail a functional change, I would have expected the updated commentary and refactor to be a separate commit—I’m wondering whether my understanding of the modus operandi is correct.

instagibbs commented at 6:21 am on April 5, 2024:

split out the moving and docstring changes, should be minimal now?

murchandamus commented at 12:16 pm on April 8, 2024:

Nit: I don’t feel strongly about it, but the change that made me notice that there were code changes in the move originally was that COINBASE_MATURITY - 1 had gotten replaced with a 99 magic number.

instagibbs commented at 1:17 pm on April 8, 2024:

bit of an annoying circular reference issue(blocktools requiring utils) so for now at least make it grep-able what the magic value should be(and more greppable)

in test/functional/rpc_packages.py:388 in 271599af6d outdated

381+        # but child is too high fee
382+        # Needed for fill_mempool
383+        self.restart_node(0, extra_args=[
384+            "-datacarriersize=100000",
385+            "-maxmempool=5",
386+        ])

murchandamus commented at 6:17 pm on April 4, 2024:

In “AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure” (271599af6d84c871154dc16c7abcadbe5ac08163):

If these node parameters generally are needed for fill_mempool to work, would it perhaps make sense to move them to the fill_mempool function?

Alternatively, the comment should perhaps be “Lower mempool limit to make it easier to fill_mempool”

instagibbs commented at 6:21 am on April 5, 2024:

I don’t want to blow away extra_args the caller may want, so took the comment suggestion for now

glozow commented at 12:53 pm on April 9, 2024:

👍 I think this interface is better if the caller knows what the requirements are but can otherwise put whatever args they want

in test/functional/rpc_packages.py:407 in 271599af6d outdated

402+        pkg_result = node.submitpackage([parent["hex"], child["hex"]], maxfeerate=DEFAULT_FEE - Decimal("0.00000001"))
403+
404+        # Child is connected even though parent is invalid and still reports fee exceeded
405+        # this implies sub-package evaluation of both entries together.
406+        assert_equal(pkg_result["package_msg"], "transaction failed")
407+        assert "mempool min fee not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]

murchandamus commented at 6:34 pm on April 4, 2024:

In “AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure” (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):

Pet-peeve nit: I suspect that the issues here is rather that the mempool min feerate is not met?

0        assert "mempool min feerate not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]

instagibbs commented at 6:00 am on April 5, 2024:

that’s the error string as it’s reported unfortunately!

in test/functional/rpc_packages.py:411 in 271599af6d outdated

406+        assert_equal(pkg_result["package_msg"], "transaction failed")
407+        assert "mempool min fee not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
408+        assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded")
409+        assert_equal(node.getrawmempool(), [])
410+
411+        # Reset minfee

murchandamus commented at 6:36 pm on April 4, 2024:

In “AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure” (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):

Did you perhaps mean:

0        # Reset maxmempool and datacarriersize, and empty mempool to reset dynamic mempool minimum feerate

?

instagibbs commented at 6:21 am on April 5, 2024:

no but this is even better, taken

in test/functional/rpc_packages.py:416 in 271599af6d outdated

408+        assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded")
409+        assert_equal(node.getrawmempool(), [])
410+
411+        # Reset minfee
412+        self.restart_node(0)
413+

murchandamus commented at 6:44 pm on April 4, 2024:

In “AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure” (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163): At this point, it might be better to split the maxfeerate and the maxburnamount tests.

instagibbs commented at 6:20 am on April 5, 2024:

done

murchandamus commented at 6:47 pm on April 4, 2024: contributor

ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4 with nits.

DrahtBot removed the label CI failed on Apr 4, 2024

instagibbs force-pushed on Apr 5, 2024

in test/functional/test_framework/util.py:504 in ac0cfcaeef outdated

496@@ -497,7 +497,15 @@ def check_node_connections(*, node, num_in, num_out):
497     assert_equal(info["connections_out"], num_out)
498 
499 def fill_mempool(test_framework, node, miniwallet):
500-        """Fill mempool until eviction."""
501+        """Fill mempool until eviction.
502+
503+        Allows for simpler testing of scenarios with minfee != minrelay
504+        Requires -datacarriersize=100000 and
505+        -maxmempool=5(or similarly small).

glozow commented at 2:14 pm on April 5, 2024:

“or similarly small” doesn’t really make sense to me. AFAICT this makes just enough transactions to go past the 5MB mempool.

instagibbs commented at 9:53 am on April 6, 2024:

removed the parenthesis

in test/functional/test_framework/util.py:502 in ac0cfcaeef outdated

496@@ -497,7 +497,15 @@ def check_node_connections(*, node, num_in, num_out):
497     assert_equal(info["connections_out"], num_out)
498 
499 def fill_mempool(test_framework, node, miniwallet):
500-        """Fill mempool until eviction."""
501+        """Fill mempool until eviction.
502+
503+        Allows for simpler testing of scenarios with minfee != minrelay

glozow commented at 2:14 pm on April 5, 2024:

nit

0        Allows for simpler testing of scenarios with minfee > minrelay

instagibbs commented at 9:53 am on April 6, 2024:

taken

in test/functional/rpc_packages.py:419 in 1d79aabed4 outdated

411@@ -411,6 +412,12 @@ def test_maxfeerate_maxburn_submitpackage(self):
412         # Reset maxmempool and datacarriersize, and empty mempool to reset dynamic mempool minimum feerate
413         self.restart_node(0)
414 
415+    def test_maxburn_submitpackage(self):
416+        node = self.nodes[0]
417+        # clear mempool
418+        deterministic_address = node.get_deterministic_priv_key().address
419+        self.generatetoaddress(node, 1, deterministic_address)

glozow commented at 2:19 pm on April 5, 2024:

Mempool is already empty?

instagibbs commented at 9:53 am on April 6, 2024:

replaced with an assert

in test/functional/rpc_packages.py:390 in 2b346ba975 outdated

385+            "-maxmempool=5",
386+        ])
387+
388+        fill_mempool(self, node, self.wallet)
389+        while node.getrawmempool() != []:
390+            self.generate(node, 1)

glozow commented at 2:21 pm on April 5, 2024:

Note that this can change the mempool min fee

instagibbs commented at 2:34 pm on April 5, 2024:

mm yeah let me make this a bit more bullet proof

glozow commented at 2:41 pm on April 5, 2024:

Well I don’t think you need get rid of the transactions here - you just want to make sure they get cleared by the end of the test right?

instagibbs commented at 9:53 am on April 6, 2024:

felt like I had a reason for this, but apparently not. At end of test I empty mempool on node restart, so removed

in test/functional/rpc_packages.py:411 in 2b346ba975 outdated

406+        assert_equal(pkg_result["package_msg"], "transaction failed")
407+        assert "mempool min fee not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
408+        assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded")
409+        assert_equal(node.getrawmempool(), [])
410+
411+        # Reset maxmempool and datacarriersize, and empty mempool to reset dynamic mempool minimum feerate

glozow commented at 2:23 pm on April 5, 2024:

Mempool is already empty

0        # Reset maxmempool, datacarriersize, and dynamic mempool minimum feerate

instagibbs commented at 9:53 am on April 6, 2024:

changed where it’s non-empty, so added persistmempool=0 and left a new comment

instagibbs force-pushed on Apr 6, 2024

in test/functional/test_framework/util.py:502 in a466686c6e outdated

496@@ -497,7 +497,15 @@ def check_node_connections(*, node, num_in, num_out):
497     assert_equal(info["connections_out"], num_out)
498 
499 def fill_mempool(test_framework, node, miniwallet):
500-        """Fill mempool until eviction."""
501+        """Fill mempool until eviction.
502+
503+        Allows for simpler testing of scenarios with floating minfee > minrelay

ismaelsadeeq commented at 11:54 am on April 8, 2024:

In a466686c6e97d47e3f4718eed0f781acb88da7fa “test: expand docstring to fill_mempool”

nit:

0        Allows for simpler testing of scenarios with floating mempoolminfee > minrelayfee

instagibbs commented at 1:17 pm on April 8, 2024:

done

in test/functional/test_framework/util.py:511 in c93f790cad outdated

506+        num_of_batches = 75
507+        # Generate UTXOs to flood the mempool
508+        # 1 to create a tx initially that will be evicted from the mempool later
509+        # 75 transactions each with a fee rate higher than the previous one
510+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
511+        # And 2 more for the package cpfp test

ismaelsadeeq commented at 12:18 pm on April 8, 2024:

In c93f790cad1558dcf0b8fb5fd0cf3a276368d656 " Move fill_mempool to util function"

There should be a commit that will first pull out these two lines from this scope to https://github.com/bitcoin/bitcoin/blob/f0794cbd405636a7f528a60f2873050b865cf7e8/test/functional/mempool_limit.py#L231

Moving fill_mempool to test_framework/util.py makes it clear that the comments do not belong to this scope.

instagibbs commented at 1:18 pm on April 8, 2024:

done, I think

in src/validation.cpp:1367 in 72425bd699 outdated

1365@@ -1366,7 +1366,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1366         // Individual modified feerate exceeded caller-defined max; abort
1367         // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust.

murchandamus commented at 12:20 pm on April 8, 2024:

Nit: Subject line in commit message should not exceed 50 characters, and definitely not be more than 72: https://cbea.ms/git-commit/#limit-50

instagibbs commented at 1:16 pm on April 8, 2024:

Made it shorter :)

in test/functional/test_framework/util.py:507 in a466686c6e outdated

503+        Allows for simpler testing of scenarios with floating minfee > minrelay
504+        Requires -datacarriersize=100000 and
505+        -maxmempool=5.
506+        It will not ensure mempools become synced as it
507+        is based on a single node and assumes -minrelaytxfee
508+        is 1 sat/vbyte.

ismaelsadeeq commented at 12:22 pm on April 8, 2024:

Maybe assert for this at the beginning of the fill_mempool

0     assert_equal(node.getnetworkinfo()['relayfee'], Decimal('0.00001000'))

instagibbs commented at 1:18 pm on April 8, 2024:

done

ismaelsadeeq commented at 3:33 pm on April 8, 2024:

looking at this again, what do you think about just restarting the node with the -datacarriersize=100000, -maxmempool=5, -minrelaytxfee=0.00001000 after filling the mempool we then restart the node with default node settings? Are there any advantage of delegating setting this to the caller? If not then this will reduce duplication of all callers of fill_mempool settings this values.

instagibbs commented at 5:52 am on April 9, 2024:

We can’t restart the node after filling the memopool without resetting the floating minfee.

I could pull in the initial restart if we knew people didn’t want any other extra_args.

Alternatively I could assert that datacarrier and maxmempool is set in extra_args?

I think documentation is enough for now, leaving as is.

murchandamus commented at 12:23 pm on April 8, 2024: contributor

ACK 052c67d4beb72d3fdf31fa5e584ea2fa12e6f69c

Please feel free to ignore nits

DrahtBot requested review from glozow on Apr 8, 2024

ismaelsadeeq commented at 12:27 pm on April 8, 2024: member

Concept ACK

I will run the new fuzz test with and without 72425bd699dd33d57794b3339f6266c4c9df443d

fill_mempool: remove subtest-specific comment 73b68bd8b4

instagibbs force-pushed on Apr 8, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 3:25 pm on April 8, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/29735/commits/4fe7d150eb3c85a6597d8fc910fe1490358197ad

I’ve reviewed 73b68….4fe7d15

I’ve Ran the fuzz test without f28338b82be9820380ef217905ae9c167164f181

And verified the crash

0Test unit written to ./crash-7fd49c80d3088f672b4bf03ab8a1d7f1aedff0aa
1Base64: q6sAIqsA/6sACQCrAAAAANr/+zUBAQENASUABAB5aKqgnQAAAAAlKv8tATUBAQENASUABAB5aKqgnQAAAAAluQAAAMP3AAAAAasAAAAAAAAA2v/7Kv8tATYBAQENASUABAB0aKKgle/f/wAA

I ran functional tests and rpc_packages.py failed.

I then cherry-picked the patch f28338b82be9820380ef217905ae9c167164f181 and run the fuzz test again, it passed successfully.

0INFO: seed corpus: files: 728 min: 1b max: 617355b total: 10276044b rss: 232Mb
1[#128](/bitcoin-bitcoin/128/)    pulse  cov: 10579 ft: 39305 corp: 108/4319b exec/s: 64 rss: 260Mb
2[#512](/bitcoin-bitcoin/512/)    pulse  cov: 11879 ft: 55511 corp: 356/67Kb exec/s: 36 rss: 283Mb
3[#925](/bitcoin-bitcoin/925/)    INITED cov: 12041 ft: 66449 corp: 523/8522Kb exec/s: 20 rss: 550Mb
4[#925](/bitcoin-bitcoin/925/)    DONE   cov: 12041 ft: 66449 corp: 523/8522Kb lim: 617355 exec/s: 20 rss: 550Mb
5Done 925 runs in 46 second(s)

Functional tests are also passing

DrahtBot requested review from murchandamus on Apr 8, 2024

Move fill_mempool to util function a3da63e8fe

in test/functional/rpc_packages.py:412 in f28338b82b outdated

407+        assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded")
408+        assert parent["txid"] not in node.getrawmempool()
409+        assert child["txid"] not in node.getrawmempool()
410+
411+        # Reset maxmempool, datacarriersize, ,reset dynamic mempool minimum feerate, and empty mempool.
412+        self.restart_node(0, extra_args=["-persistmempool=0"])

glozow commented at 12:50 pm on April 9, 2024:

This should be in the extra args above to avoid writing mempool.dat.

Currently the test is fine, but this is because the new datacarriersize causes all the transactions to be rejected.

instagibbs commented at 12:56 pm on April 9, 2024:

moved and added assertion of empty mempool after restart

DrahtBot requested review from glozow on Apr 9, 2024

fill_mempool: assertions and docsctring update f3aa5bd5eb

AcceptMultipleTransactions: Fix workspace client_maxfeerate

If we do not set the Failure for the workspace when
there is a client_maxfeerate related error, we hit
an Assume() to the contrary. Properly set it.

91d7d8f22a

fuzz: Add coverage for client_maxfeerate 4ba1d0b553

instagibbs force-pushed on Apr 9, 2024

glozow commented at 3:19 pm on April 9, 2024: member

reACK 4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46

DrahtBot requested review from ismaelsadeeq on Apr 9, 2024

theStack approved

theStack commented at 8:28 am on April 11, 2024: contributor

ACK 4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46

ismaelsadeeq commented at 9:09 am on April 11, 2024: member

re-ACK https://github.com/bitcoin/bitcoin/commit/4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46 via diff

glozow merged this on Apr 11, 2024

glozow closed this on Apr 11, 2024

Pttn referenced this in commit 4e744be34f on Apr 13, 2024

Fabcien referenced this in commit f7a23c763c on Jul 17, 2024

AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure #29735

Code Coverage

Reviews

Conflicts