policy / validation: CPFP fee bumping within packages #24152
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Concept ACK
left a few comments below
|
Concept ACK Built on MacOS Big Sur, ran unit tests (diagrams from PR review club are helpful here illustrating each unit test). Ideally I'd play around with these but just read through and ran them today. Didn't run the fuzz tests but the fuzzer seems to be failing on the CI. I asked about DoS vectors for CPFP in today's PR review club as that seems the greatest hurdle to get over for updated RBF rules and package RBF but seem to be less of a concern with CPFP (?). Not sure whether the CPFP carve out rule needs to be tweaked when CPFPing within a package? Great to see this series of PRs progress. |
glozow
force-pushed
the
package-cpfp
branch
2 times, most recently
from
9ebe881 to
9ff00ab
Compare
|
Thanks @ariard @mzumsande @LarryRuane @michaelfolkson! Addressed your review comments. Clarified the documentation, adjusted the fuzz/tx_pool, and added the test for negative prioritisation. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
77202f0 [doc] package deduplication (glozow) d35a3cb [doc] clarify inaccurate comment about replacements paying higher feerate (glozow) 5ae187f [validation] look up transaction by txid (glozow) Pull request description: - Use txid, not wtxid, for `mempool.GetIter()`: bitcoin/bitcoin#22674 (comment) - Fix a historically inaccurate comment about RBF during the refactors: bitcoin/bitcoin#22855 (comment) - Add a section about package deduplication to policy/packages.md: bitcoin/bitcoin#24152 (comment) and bitcoin/bitcoin#24152 (comment) (I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from #24152) ACKs for top commit: t-bast: LGTM, ACK bitcoin/bitcoin@77202f0 darosior: ACK 77202f0 LarryRuane: ACK 77202f0 Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696
|
Rebased |
77202f0 [doc] package deduplication (glozow) d35a3cb [doc] clarify inaccurate comment about replacements paying higher feerate (glozow) 5ae187f [validation] look up transaction by txid (glozow) Pull request description: - Use txid, not wtxid, for `mempool.GetIter()`: bitcoin#22674 (comment) - Fix a historically inaccurate comment about RBF during the refactors: bitcoin#22855 (comment) - Add a section about package deduplication to policy/packages.md: bitcoin#24152 (comment) and bitcoin#24152 (comment) (I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from bitcoin#24152) ACKs for top commit: t-bast: LGTM, ACK bitcoin@77202f0 darosior: ACK 77202f0 LarryRuane: ACK 77202f0 Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696
doc/policy/packages.md
Outdated
There was a problem hiding this comment.
s/backwards/backward/g ; s/compatibile/compatible/g (though ofc compatibility exists) ?
|
Fixed the fuzzer issue, added a commit for quitting early if transactions fail for reasons other than policy or missing inputs. |
|
pinging @ariard @darosior @LarryRuane @mzumsande if you have time to take another look? :) |
ariard
left a comment
ariard
left a comment
There was a problem hiding this comment.
Code Review ACK a7017507
Modifications since last review are typying arguments in AcceptMultipleTransactions and quit_early new a7017507 commit.
|
Out of scope for now but dropped a message on the gist itself https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a I see no reason for this(possibly wrongly perceived) shortfall to stop incremental improvements |
Will respond on the gist. I don't think it's relevant to packages actually. |
Thanks, suggestion for future, make the rebase and address-review-comments two separate force-pushes for easier review. |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
code review ACK 157a7bb547abc90a8bbb595cdee8eaac9c3300c9
docs are clear re:design, tests look fairly comprehensive, nice step forward
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Reviewed again and it looks good to me, just two questions before I ACK:
Could this introduce some weird behavior for the testmempoolaccept RPC? That RPC bypasses AcceptPackage() and calls AcceptMultipleTransactions() directly, which now has logic to calculate a package feerate (and possibly return a related error) which only makes sense for the special case of a "exactly one child and its parents" package that AcceptPackage() ensures.
Another question about RBF below.
This allows CPFP within a package prior to submission to mempool.
This avoids "parents pay for children" and "siblings pay for siblings" behavior, since package feerate is calculated with totals and is topology-unaware. It also ensures that package validation never causes us to reject a transaction that we would have otherwise accepted in single-tx validation.
Package validation policy only differs from individual policy in its evaluation of feerate. Minimize DoS surface; don't validate all over again if we know the result will be the same.
|
In response to @mzumsande's (very good) questions, I have made these 2 changes:
Also addressed @LarryRuane's #24152 (comment) which I forgot to address last time. |
|
changes are the aforementioned reACK 9bebf35 |
|
Code review ACK 9bebf35 |
Regression in bitcoin#24152.
|
Post-Merge Review ACK 9bebf35 Changes since last ACK are turning |
Regression in bitcoin#24152.
author MarcoFalke <[email protected]> 1649237525 +0200 committer russeree <[email protected]> 1650013843 -0700 parent 10f629e author MarcoFalke <[email protected]> 1649237525 +0200 committer russeree <[email protected]> 1650013815 -0700 parent 10f629e author MarcoFalke <[email protected]> 1649237525 +0200 committer russeree <[email protected]> 1650013794 -0700 ci: Build all optional tools in tidy task lint: remove boost::bind linter I don't think we need to maintain a linter for reintroducing boost::bind at this point. doc: Convert remaining comments to clang-tidy format [docs] package feerate [packages/policy] use package feerate in package validation This allows CPFP within a package prior to submission to mempool. [validation] try individual validation before package validation This avoids "parents pay for children" and "siblings pay for siblings" behavior, since package feerate is calculated with totals and is topology-unaware. It also ensures that package validation never causes us to reject a transaction that we would have otherwise accepted in single-tx validation. [unit test] package feerate and package cpfp [validation] don't package validate if not policy or missing inputs Package validation policy only differs from individual policy in its evaluation of feerate. Minimize DoS surface; don't validate all over again if we know the result will be the same. lint: remove qt SIGNAL/SLOT lint I think we are past the point where we need to lint for this, the CPU can probably be better utilized. refactor: Remove deduplication of data in rollingbloom bench lint: codespell 2.1.0 lint: flake8 4.0.1 lint: mypy 0.942 refactor: fixup named args in txpackage tests Regression in bitcoin#24152. Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive Add DEBUG_LOCKCONTENTION documentation to the developer notes Squash Squashed All Previous Commits RPC: Switch getblockfrompeer back to standard param name blockhash This commit partially reverts 923312f. Update RPC argument and field naming guideline in developer notes Co-authored-by: laanwj <[email protected]> build: fix MSVC build after subtree update Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: Aaron Clauson <[email protected]> build: remove --enable-experimental from libsecp256k1 configure build: remove some no-longer-needed var unexporting from configure key: use secp256k1_schnorrsig_sign32 over deprecated secp256k1_schnorrsig_sign The renaming occured in bitcoin-core/secp256k1#1089. Squased lint changes - added features Squashed to reduce spam - added features Loading ASN directory from a .tsv file. Added the use of a GZIP compressed ip -> ASN file. LINT Fixes - 7 words Fixed comparison to none using 'is' instead of '==' Fixed unintended changes to readme.md whitespace correction restored generate-seeds.py restored makeseeds.py Fixed a missing return type within exception Removed unused variable and if __main__ Updated to fetch and included legacy failover LINT fixes LINT fixes Squash - Too many commits Python include dns.resolver Failover implementation complete Removed DNS resolver Revert changes refactor: Remove deduplication of data in rollingbloom bench lint: mypy 0.942 refactor: fixup named args in txpackage tests Regression in bitcoin#24152. Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive Add DEBUG_LOCKCONTENTION documentation to the developer notes Squash Squashed All Previous Commits RPC: Switch getblockfrompeer back to standard param name blockhash This commit partially reverts 923312f. Update RPC argument and field naming guideline in developer notes Co-authored-by: laanwj <[email protected]> build: fix MSVC build after subtree update Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: Aaron Clauson <[email protected]> build: remove --enable-experimental from libsecp256k1 configure build: remove some no-longer-needed var unexporting from configure key: use secp256k1_schnorrsig_sign32 over deprecated secp256k1_schnorrsig_sign The renaming occured in bitcoin-core/secp256k1#1089. Squased lint changes - added features Squashed to reduce spam - added features Loading ASN directory from a .tsv file. Added the use of a GZIP compressed ip -> ASN file. LINT Fixes - 7 words Fixed comparison to none using 'is' instead of '==' Fixed unintended changes to readme.md whitespace correction restored generate-seeds.py restored makeseeds.py Fixed a missing return type within exception Removed unused variable and if __main__ Updated to fetch and included legacy failover LINT fixes LINT fixes Squash - Too many commits Python include dns.resolver Failover implementation complete Removed DNS resolver Revert changes Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive Add DEBUG_LOCKCONTENTION documentation to the developer notes Squash Squashed All Previous Commits RPC: Switch getblockfrompeer back to standard param name blockhash This commit partially reverts 923312f. Update RPC argument and field naming guideline in developer notes Co-authored-by: laanwj <[email protected]> build: fix MSVC build after subtree update Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: Aaron Clauson <[email protected]> build: remove --enable-experimental from libsecp256k1 configure build: remove some no-longer-needed var unexporting from configure key: use secp256k1_schnorrsig_sign32 over deprecated secp256k1_schnorrsig_sign The renaming occured in bitcoin-core/secp256k1#1089. Squased lint changes - added features Squashed to reduce spam - added features Loading ASN directory from a .tsv file. Added the use of a GZIP compressed ip -> ASN file. LINT Fixes - 7 words Fixed comparison to none using 'is' instead of '==' Fixed unintended changes to readme.md whitespace correction restored generate-seeds.py restored makeseeds.py Fixed a missing return type within exception Removed unused variable and if __main__ Updated to fetch and included legacy failover LINT fixes LINT fixes Squash - Too many commits Python include dns.resolver Failover implementation complete Removed DNS resolver Revert changes RPC: Switch getblockfrompeer back to standard param name blockhash This commit partially reverts 923312f. Update RPC argument and field naming guideline in developer notes Co-authored-by: laanwj <[email protected]> build: fix MSVC build after subtree update Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: Aaron Clauson <[email protected]> build: remove --enable-experimental from libsecp256k1 configure build: remove some no-longer-needed var unexporting from configure key: use secp256k1_schnorrsig_sign32 over deprecated secp256k1_schnorrsig_sign The renaming occured in bitcoin-core/secp256k1#1089. test: compare `/chaininfo` response with `getblockchaininfo` RPC test: use MiniWallet for feature_fee_estimation.py This test can now be run even with the Bitcoin Core wallet disabled. Converted lint-python-mutable-default-parameters.sh to python Change permission Change argument so that it's compatiable with python 3.6 Change comment to docstring Remove .split, .append, .extend calls. Remove 'output' variable assignment build: Do not define `PROVIDE_FUZZ_MAIN_FUNCTION` macro unconditionally builder-keys: Add will8clark gui: add FormatPeerAge() utility helper Co-authored-by: randymcmillan <[email protected]> gui: add Age column to peers tab Co-authored-by: Jon Atack <[email protected]> gui: peersWidget - ResizeToContents Age and IP/Netmask columns Co-authored-by: Hennadii Stepanov <[email protected]> gui: add test runner summary gui: count test failures in test runner summary gui, refactor: rename fInvalid to num_test_failures in test_main.cpp qt: Fix headers This change is preparation for Qt 6, and it fixes an experimental build with Qt 6.2.4. qt: Use `|` instead of `+` for key modifiers This change is preparation for Qt 6 where `+` has been deprecated, and it fixes an experimental build with Qt 6.2.4. qt: Update deprecated enum value This change is preparation for Qt 6, and it fixes an experimental build with Qt 6.2.4. The `Qt::ItemIsTristate` value has been deprecated since 5.6.0 (see ae8406d82f541f6d9112bdac192e5e4e114d56aa upstream commit). print `(none)` if no warnings in -getinfo build, refactor: Drop useless `call` Make function util, refactor: Add UNIQUE_NAME helper macro This change replaces repetitive code with a helper macro. Replace uint256 specific implementations of base_uint::GetHex() and base_uint::SetHex() with proper ones that don't depend on uint256 and replace template methods instantiations of base_uint with template class instantiation guix: fix GCC 10.3.0 + mingw-w64 setjmp/longjmp issues This commit backports a patch to the GCC 10.3.0 we build for Windows cross-compilation in Guix. The commit has been backported to the GCC releases/gcc-10 branch, but hasn't yet made it into a release. The patch corrects a regression from an earlier GCC commit, see: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=357c4350680bf29f0c7a115424e3da11c53b5582 and https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=074226d5aa86cd3de517014acfe34c7f69a2ccc7, related to the way newer versions of mingw-w64 implement setjmp/longjmp. Ultimately this was causing a crash for us when Windows users were viewing the network traffic tab inside the GUI. After some period, long enough that a buffer would need reallocating, a call into FreeTypes gray_record_cell() would result in a call to ft_longjmp (longjmp), which would then trigger a crash. Fixes: bitcoin-core/gui#582. See also: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=e8d1ca7d2c344a411779892616c423e157f4aea8. https://bugreports.qt.io/browse/QTBUG-93476. doc: Remove fee delta TODO from txmempool.cpp net: remove non-blocking bool from interface lint: Convert lint-logs.sh to Python test: determine path to `bitcoin-util` in test framework The path is stored in `self.options.bitcoinutil`, points to `src/bitcoin-util` by default and can be overrided with the `BITCOINUTIL` environment variable. test: add `is_bitcoin_util_compiled` helper test: add test for signet miner script depends: Add file-based logging for individual packages ci: Make log verbose in error case only This change silences depends build using LOG=1. doc: Add pre-splitoff translation update to release-process.md
There was a problem hiding this comment.
I think the package feerate check shouldn't be in AcceptMultipleTransactions, since it doesn't necessarily take a child-with-parents package.
(sorry, i know i'm late to the party)
EDIT: aarg. I was reviewing an older branch which didn't have the fix for m_package_feerates in PackageTestAccept (#24152 (comment))... Sorry for the noise.
| const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); | ||
| BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value()); | ||
| BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0}); | ||
| BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate, |
There was a problem hiding this comment.
nit: this last check is redundant with the previous since expected_feerate is already instantiated to 0
| // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. | ||
| // For transactions consisting of exactly one child and its parents, it suffices to use the | ||
| // package feerate (total modified fees / total virtual size) to check this requirement. | ||
| const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, |
There was a problem hiding this comment.
This assumes that AcceptMultipleTransactions always takes proper packages and that we can safely consider the aggregated feerate of the bunch of transactions. It's not the case.
EDIT: i was reviewing an older version of this branch which was missing a fix.... (#24152 (review))
This patch demonstrates how this makes testmempoolaccept accept invalid transactions (by considering the package feerate for unrelated transactions):
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index d3961e753d..f8dd380ee5 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -333,6 +333,15 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
maxfeerate=0,
)
+ self.log.info("Don't consider package fees between unrelated transactions")
+ txs = [
+ self.wallet.create_self_transfer(fee_rate=0, mempool_valid=False)['tx'],
+ self.wallet.create_self_transfer()['tx'],
+ ]
+ res = self.nodes[0].testmempoolaccept([tx.serialize().hex() for tx in txs])
+ # The first tx has a feerate of 0. No child pay for it. It should be rejected.
+ assert not res[0]["allowed"]
+
if __name__ == '__main__':
MempoolAcceptanceTest().main()
12 participants
Part of #22290, aka Package Mempool Accept.
This enables CPFP fee bumping in child-with-unconfirmed-parents packages by introducing package feerate (total modified fees divided by total virtual size) and using it in place of individual feerate. We also always validate individual transactions first to avoid incentive-incompatible policies like "parents pay for children" or "siblings pay for siblings" behavior.