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

instagibbs · 2024-03-26T14:41:47Z

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 · 2024-03-26T14:41:50Z

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.

glozow

Concept ACK, verified that on master the test fails since the individual result wasn't populated and is "Valid"

test/functional/test_framework/util.py

test/functional/rpc_packages.py

src/validation.cpp

instagibbs · 2024-03-27T13:31:09Z

Added some fuzz coverage which would have likely caught this

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

glozow

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

test/functional/test_framework/util.py

test/functional/rpc_packages.py

glozow

ACK 14c86ba

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

murchandamus

ACK 14c86ba with nits.

murchandamus · 2024-04-04T18:08:35Z

test/functional/test_framework/util.py

+        test_framework.generate(miniwallet, 1 + (num_of_batches * tx_batch_size))
+
+        # Mine 99 blocks so that the UTXOs are allowed to be spent
+        test_framework.generate(node, 99)


In "Move fill_mempool to util function" (cc770e4):

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.

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

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.

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)

test/functional/rpc_packages.py

test/functional/test_framework/util.py

test/functional/rpc_packages.py

murchandamus

ACK 052c67d

Please feel free to ignore nits

murchandamus · 2024-04-08T12:16:23Z

test/functional/test_framework/util.py

+        test_framework.generate(miniwallet, 1 + (num_of_batches * tx_batch_size))
+
+        # Mine 99 blocks so that the UTXOs are allowed to be spent
+        test_framework.generate(node, 99)


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.

src/validation.cpp

ismaelsadeeq

Concept ACK

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

ismaelsadeeq · 2024-04-08T11:54:10Z

test/functional/test_framework/util.py

-        """Fill mempool until eviction."""
+        """Fill mempool until eviction.
+
+        Allows for simpler testing of scenarios with floating minfee > minrelay


In a466686 "test: expand docstring to fill_mempool"

nit:

Suggested change

Allows for simpler testing of scenarios with floating minfee > minrelay

Allows for simpler testing of scenarios with floating mempoolminfee > minrelayfee

ismaelsadeeq · 2024-04-08T12:18:14Z

test/functional/test_framework/util.py

+        # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
+        # And 2 more for the package cpfp test


In c93f790 " Move fill_mempool to util function"

There should be a commit that will first pull out these two lines from this scope to

bitcoin/test/functional/mempool_limit.py

Line 231 in f0794cb

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

done, I think

ismaelsadeeq · 2024-04-08T12:22:05Z

test/functional/test_framework/util.py

+        It will not ensure mempools become synced as it
+        is based on a single node and assumes -minrelaytxfee
+        is 1 sat/vbyte.


Maybe assert for this at the beginning of the fill_mempool

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

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.

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.

ismaelsadeeq

ACK 4fe7d15

I've reviewed 73b68....4fe7d15

I've Ran the fuzz test without f28338b

And verified the crash

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

I ran functional tests and rpc_packages.py failed.

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

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

Functional tests are also passing

glozow · 2024-04-09T12:50:32Z

test/functional/rpc_packages.py

+        assert child["txid"] not in node.getrawmempool()
+
+        # Reset maxmempool, datacarriersize, ,reset dynamic mempool minimum feerate, and empty mempool.
+        self.restart_node(0, extra_args=["-persistmempool=0"])


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.

moved and added assertion of empty mempool after restart

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.

glozow

reACK 4ba1d0b

theStack

ACK 4ba1d0b

ismaelsadeeq

re-ACK 4ba1d0b via diff

glozow added the Validation label Mar 26, 2024

instagibbs mentioned this pull request Mar 26, 2024

Cluster size 2 package rbf #28984

Open

glozow reviewed Mar 27, 2024

View reviewed changes

test/functional/test_framework/util.py Outdated Show resolved Hide resolved

test/functional/test_framework/util.py Show resolved Hide resolved

test/functional/rpc_packages.py Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch from 5ae4dce to 4f8407c Compare March 27, 2024 12:55

This was referenced Mar 27, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

kernel: Handle fatal errors through return values #29642

Draft

glozow mentioned this pull request Mar 28, 2024

p2p: opportunistically accept 1-parent-1-child packages #28970

Merged

glozow reviewed Apr 3, 2024

View reviewed changes

test/functional/test_framework/util.py Outdated Show resolved Hide resolved

test/functional/rpc_packages.py Outdated Show resolved Hide resolved

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch from b057d2b to 14c86ba Compare April 4, 2024 08:24

glozow reviewed Apr 4, 2024

View reviewed changes

DrahtBot added the CI failed label Apr 4, 2024

glozow mentioned this pull request Apr 4, 2024

Package Relay Project Tracking #27463

Open

53 tasks

murchandamus reviewed Apr 4, 2024

View reviewed changes

DrahtBot removed the CI failed label Apr 4, 2024

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch from 14c86ba to 1d79aab Compare April 5, 2024 06:20

glozow reviewed Apr 5, 2024

View reviewed changes

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch from 1d79aab to 052c67d Compare April 6, 2024 09:53

murchandamus reviewed Apr 8, 2024

View reviewed changes

DrahtBot requested a review from glozow April 8, 2024 12:23

ismaelsadeeq reviewed Apr 8, 2024

View reviewed changes

fill_mempool: remove subtest-specific comment

73b68bd

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch 2 times, most recently from 4aa47e7 to 4fe7d15 Compare April 8, 2024 13:16

ismaelsadeeq approved these changes Apr 8, 2024

View reviewed changes

DrahtBot requested a review from murchandamus April 8, 2024 15:25

DrahtBot mentioned this pull request Apr 8, 2024

improve MallocUsage() accuracy #28531

Draft

theStack mentioned this pull request Apr 9, 2024

test: p2p: add test for rejected tx request logic (m_recent_rejects filter) #29827

Closed

Move fill_mempool to util function

a3da63e

glozow reviewed Apr 9, 2024

View reviewed changes

DrahtBot requested a review from glozow April 9, 2024 12:50

instagibbs added 3 commits April 9, 2024 14:53

fill_mempool: assertions and docsctring update

f3aa5bd

AcceptMultipleTransactions: Fix workspace client_maxfeerate

91d7d8f

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.

fuzz: Add coverage for client_maxfeerate

4ba1d0b

instagibbs force-pushed the 2024-03-fix-multitx-maxfee branch from 4fe7d15 to 4ba1d0b Compare April 9, 2024 12:54

glozow reviewed Apr 9, 2024

View reviewed changes

DrahtBot requested a review from ismaelsadeeq April 9, 2024 15:20

theStack approved these changes Apr 11, 2024

View reviewed changes

ismaelsadeeq reviewed Apr 11, 2024

View reviewed changes

glozow merged commit bdb33ec into bitcoin:master Apr 11, 2024
16 checks passed

Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024

Merge bitcoin#29735

4e744be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

instagibbs commented Mar 26, 2024 •

edited

DrahtBot commented Mar 26, 2024 •

edited

glozow left a comment

instagibbs commented Mar 27, 2024 •

edited

glozow left a comment

glozow left a comment

murchandamus left a comment

murchandamus Apr 4, 2024

instagibbs Apr 5, 2024

murchandamus Apr 8, 2024

instagibbs Apr 8, 2024

murchandamus left a comment

murchandamus Apr 8, 2024

ismaelsadeeq left a comment

ismaelsadeeq Apr 8, 2024

instagibbs Apr 8, 2024

ismaelsadeeq Apr 8, 2024

instagibbs Apr 8, 2024

ismaelsadeeq Apr 8, 2024

instagibbs Apr 8, 2024

ismaelsadeeq Apr 8, 2024

instagibbs Apr 9, 2024

ismaelsadeeq left a comment

glozow Apr 9, 2024

instagibbs Apr 9, 2024

glozow left a comment

theStack left a comment

ismaelsadeeq left a comment

	Allows for simpler testing of scenarios with floating minfee > minrelay
	Allows for simpler testing of scenarios with floating mempoolminfee > minrelayfee

		# And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
		# And 2 more for the package cpfp test

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

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

Conversation

instagibbs commented Mar 26, 2024 • edited

DrahtBot commented Mar 26, 2024 • edited

Code Coverage

Reviews

Conflicts

glozow left a comment

Choose a reason for hiding this comment

instagibbs commented Mar 27, 2024 • edited

glozow left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

instagibbs commented Mar 26, 2024 •

edited

DrahtBot commented Mar 26, 2024 •

edited

instagibbs commented Mar 27, 2024 •

edited