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 14c86ba721e1a208c88ada133ba9e90e24724ea4

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

murchandamus

ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4 with nits.

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

test/functional/test_framework/util.py

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.

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 052c67d4beb72d3fdf31fa5e584ea2fa12e6f69c

Please feel free to ignore nits

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

test/functional/test_framework/util.py

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 72425bd699dd33d57794b3339f6266c4c9df443d

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

test/functional/test_framework/util.py

In a466686c6e97d47e3f4718eed0f781acb88da7fa "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

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

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

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 f28338b82be9820380ef217905ae9c167164f181

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 f28338b82be9820380ef217905ae9c167164f181 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

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

Summary: This is a partial backport of [[bitcoin/bitcoin#29735 | core#29735]], consisting of the first 3 commits. The remaining parts of the PR fix a bug that has not been introduced (dependency not backported yet). Test Plan: ./test/functional/test_runner.py mempool_limit Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D16486

glozow added the Validation label Mar 26, 2024

instagibbs mentioned this pull request Mar 26, 2024

Cluster size 2 package rbf #28984

Merged

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 Outdated Show resolved Hide resolved

test/functional/rpc_packages.py Outdated 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

57 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

Closed

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

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

Merge bitcoin#29735

4e744be

bitcoin locked and limited conversation to collaborators Sep 9, 2025

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

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

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

Uh oh!

Conversation

instagibbs commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

instagibbs commented Mar 26, 2024 •

edited

Loading

DrahtBot commented Mar 26, 2024 •

edited

Loading

instagibbs commented Mar 27, 2024 •

edited

Loading