New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc/validation: enable packages through testmempoolaccept #20833
Conversation
Thanks for working on this. Strong Concept ACK! |
Concept ACK and approach ACK. |
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. |
43588c4
to
79e5512
Compare
5th time's the charm I guess |
Concept ACK :) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some nits in the first commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I've reviewed the functional and unit tests and they look great! Well done doing that in a way that doesn't require the wallet - very thorough
I think soon test/functional/rpc_packages.py
can be done with MiniWallet
and get rid of some code. It would probably need #20889 though
bc we'd have to catch some exceptions where we expect from_node.testmempoolaccept
to fail. Anyways I wouldn't expect that to be done in this PR or delay anything, tests look great as is
Key functionality = a transaction with UTXOs not present in UTXO set or mempool can be fully validated instead of being considered an orphan.
Maximum number of transactions allowed in a package is 25, equal to the default mempool descendant limit: if a package has more transactions than this, either it would fail default mempool descendant limit or the transactions don't all have a dependency relationship (but then they shouldn't be in a package together). Same rationale for 101KvB virtual size package limit. Note that these policies are only used in test accepts so far.
Only allow "packages" with no conflicts, sorted in order of dependency, and no more than 25 for now. Note that these groups of transactions don't necessarily need to adhere to some strict definition of a package or have any dependency relationships. Clients are free to pass in a batch of 25 unrelated transactions if they want to.
930c6d3
to
13650fe
Compare
Thanks @ariard for reminding me about the My proposal is: If a transaction in the package exceeds maxfeerate, the "reject-reason" is "max-fee-exceeded" and all subsequent transactions have blank validation results (i.e. only "txid" and "wtxid" fields). This is the same as what we do if a transaction fails validation and is still compatible with the API on master. To respond to @mzumsande's suggestion in that comment thread:
In the future, yes. I plan to open a followup PR to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 13650fe
A few minor comments inline. A couple more:
- The commit log for 2ef1879 ([validation] package validation for test accepts) refers to
CoinsViewTemporary
, which has now been removed. - maybe move the [policy] detect unsorted packages to before the [rpc] allow multiple txns in testmempoolaccept (so you don't have to add tests and then change them later)
AssertLockHeld(pool.cs); | ||
assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); | ||
|
||
CBlockIndex* tip = active_chainstate.m_chain.Tip(); | ||
assert(tip != nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Doesn't necessarily need to be in this PR - can be done in a follow-up) it'd be nice to make tip
a reference, to better communicate that this function can't be called without a CBlockIndex
.
@@ -515,7 +515,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) | |||
LockPoints lp = it->GetLockPoints(); | |||
assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); | |||
bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); | |||
if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) || !CheckSequenceLocks(active_chainstate, *this, tx, flags, &lp, validLP)) { | |||
CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this); | |
CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this); |
@@ -494,11 +585,20 @@ class MemPoolAccept | |||
*/ | |||
std::vector<COutPoint>& m_coins_to_uncache; | |||
const bool m_test_accept; | |||
/** Disable BIP125 RBFing; disallow all conflicts with mempool transactions. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a boolean for disallow_thing
where the double negative disallow_thing == false
means "thing is allowed" is probably not as clear as a allow_thing
boolean where allow_thing == true
means "thing is allowed".
I also agree that "replacement" is probably better than "conflicts" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way max-fee-exceeded
is handled now. Some comments below, feel free to ignore if too nitty.
// We will check transaction fees we iterate through txns in order. If any transaction fee | ||
// exceeds maxfeerate, we will keave the rest of the validation results blank, because it | ||
// doesn't make sense to return a validation result for a transaction if its ancestor(s) would | ||
// not be submitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fees while we iterate, keave->leave
}, | ||
RPCResult{ | ||
RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" | ||
"Length is exactly one for now.", | ||
"Returns results for each transaction in the same order they were passed in.\n" | ||
"It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "if another transaction failed", because it's not necessarily an earlier transaction? (in the case where validation terminates early, the culprit could also be a later transaction)
{ | ||
{RPCResult::Type::OBJ, "", "", | ||
{ | ||
{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, | ||
{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"}, | ||
{RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"}, | ||
{RPCResult::Type::STR, "package-error", "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."}, | ||
{RPCResult::Type::BOOL, "allowed", "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after maxfeerate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying I should add a space after maxfeerate or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant after "maxfeerate." and before the next sentence, it should show when displaying the rpc help.
/** | ||
* Multiple transaction acceptance. Transactions may or may not be interdependent, | ||
* but must not conflict with each other. Parents must come before children if any | ||
* dependencies exist, otherwise a TX_MISSING_INPUTS error will be returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the last commit, the error will be "package-not-sorted".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 13650fe
Overall code sounds robust enough for now. I did check again test coverage, both functional/unit and it seems exhaustive. I think we can still argue on few improvements around comments but not blockers IMO.
My proposal is: If a transaction in the package exceeds maxfeerate, the "reject-reason" is "max-fee-exceeded" and all subsequent transactions have blank validation results (i.e. only "txid" and "wtxid" fields). This is the same as what we do if a transaction fails validation and is still compatible with the API on master.
That's a good-enough API for now and I agree with your proposed follow-up to calculate and evaluate the aggregated package feerate.
Other follow-ups I do have in mind :
- weight units or vbytes : rpc/validation: enable packages through testmempoolaccept #20833 (comment)
- mempool ancestor/descendants limit for packages (mempool/validation: mempool ancestor/descendant limits for packages #21800)
- package-policy checks encapsulation : rpc/validation: enable packages through testmempoolaccept #20833 (comment) (mostly motivated to have a future shared library
libtxstandardness
a lalibbitcoinconsensus
, marrying well with [rfc] add option to bypass contextual timelocks in testmempoolaccept? #21413)
What else ? I don't think ordering really matter.
"\nThis checks if the transaction violates the consensus or policy rules.\n" | ||
"\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n" | ||
"\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n" | ||
"\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think a more descriptive behavior would be "if one transaction fails, remaining transactions are not submitted for validation". See document L1146 in src/validation.cpp "Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished".
if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) { | ||
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); | ||
const UniValue raw_transactions = request.params[0].get_array(); | ||
if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the check on MAX_PACKAGE_COUNT
is duplicated into AcceptMultipleTransactions
. I can understand the rational of not taking the cs_main
lock but I've a preference to keep all package policy checks in one place. That would also avoid linking packages.h
in rawtransaction.cpp
(assuming we hardcode the package limit in RPC doc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of this as a distinct check, actually. We defined the testmempoolaccept
API to be "maximum 25 transactions" and will return a JSONRPC error here because the user violated our API, whereas AcceptMultipleTransactions()
is applying package policies.
UniValue rpc_result(UniValue::VARR); | ||
// We will check transaction fees we iterate through txns in order. If any transaction fee | ||
// exceeds maxfeerate, we will keave the rest of the validation results blank, because it | ||
// doesn't make sense to return a validation result for a transaction if its ancestor(s) would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: transaction result
protected: | ||
const CTxMemPool& mempool; | ||
|
||
public: | ||
CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); | ||
bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; | ||
/** Add the coins created by this transaction. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Temporarily add the coins created by this transaction until package processing to which it belongs is over" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a more descriptive comment in #22084
// Make the coins created by this transaction available for subsequent transactions in the | ||
// package to spend. Since we already checked conflicts in the package and RBFs are | ||
// impossible, we don't need to track the coins spent. Note that this logic will need to be | ||
// updated if RBFs in packages are allowed in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "mempool transaction replacement by packages".
Otherwise it lets suggest we apply RBF inside a currently-processing package. Also I think the replacement policy we'll end up for packages is going to be far different than BIP 125, so instead of mentioning RBF, I would say simply replacement for now.
* Atomically test acceptance of a package. If the package only contains one tx, package rules still apply. | ||
* @param[in] txns Group of transactions which may be independent or contain | ||
* parent-child dependencies. The transactions must not conflict, i.e. | ||
* must not spend the same inputs, even if it would be a valid BIP125 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit confusing, one might wonder if you're aiming to in-package conflict or mempool transactions conflicted by package. I would suggest another formulation : "The package transactions must not conflict, i.e must not spend the same inputs. They must not replace mempool transaction, even if it's valid under BIP125 rules."
Code review re-ACK 13650fe |
ee862d6 MOVEONLY: context-free package policies (glozow) 5cac95c disallow_mempool_conflicts -> allow_bip125_replacement and check earlier (glozow) e8ecc62 [refactor] comment/naming improvements (glozow) 7d91442 [rpc] reserve space in txns (glozow) 6c5f19d [package] static_assert max package size >= max tx size (glozow) Pull request description: various followups from #20833 ACKs for top commit: jnewbery: utACK ee862d6 ariard: Code Review ACK ee862d6 Tree-SHA512: 96ecb41f7bbced84d4253070f5274b7267607bfe4033e2bb0d2f55ec778cc41e811130b6321131e0418b5835894e510a4be8a0f822bc9d68d9224418359ac837
This PR enables validation dry-runs of packages through the
testmempoolaccept
RPC. The expectation is that the results returned fromtestmempoolaccept
are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished"MempoolAcceptResult
s for transactions that weren't fully validated. The API for 1 transaction stays the same.Motivation:
testmempoolaccept
#18480.testmempoolaccept
to handle client'smax_raw_tx_fee
check failure #21074 by clarifying the "allowed" key.There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion here