Picking up #21413 (labeled "Up for grabs").
This PR adds a new option (bypass_timelocks) to RPC testmempoolaccept that skips CheckFinalTxAtTip and CheckSequenceLocksAtTip, i.e. transaction nLocktime and nSequence will not be evaluated according to the current block height / time .
Script checks are still done as-is, so if the transaction itself has an error, it will still fail.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
MemPoolBypass by w0xlt){Dump,Load}Mempool from ArgsManager by dongcarl)CheckSequenceLocksAtTip() by hebasto)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.
Thanks for picking this up. I just realized I probably should have left a comment summarizing what work needs to be done. Not sure if you saw #21413 (comment)? We'll want to add (1) an option to override relative timelocks between transactions passed in, (2) a param for what MTP/height to mock, either for the whole thing or at each transaction. This would also require more tests, i.e. utilizing the new options to ensure it works beyond not breaking things.
This greatly increases the scope.
It might be better to break down each option into a specific PR.
How could testmempoolaccept simulate a specific block height, for example?
100 | - "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"}, 101 | + {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", 102 | + { 103 | + {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, 104 | + "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"}, 105 | + {"bypass_timelocks", RPCArg::Type::BOOL, RPCArg::Default{false}, "Don't enforce BIP68 sequence locks and timelocks. Don't use unless you know what you're doing!\n"}
Of course, I think this would benefit from something like standardness-sanitization in doc/policy explaining how to use the new API to an extended set of L2 developers with few use-cases.
241 | @@ -242,23 +242,26 @@ struct PackageMempoolAcceptResult 242 | * It is also used to determine when the entry expires. 243 | * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits. 244 | * @param[in] test_accept When true, run validation checks but don't submit to mempool. 245 | - * 246 | + * @param[in] bypass_timelocks When true (test_accept must also be true), don't enforce timelock 247 | + * rules BIP65 and BIP112.
I think it should say BIP68. The consensus-enforcement of relative timelock is described there, BIP112 only describes how to restrain script path execution in function of spent inputs maturity thanks to CSV. Following the same logic it should also say that the nLocktime are not enforced, without ref to BIP65.
Concept ACK
If you're interested by the whole context, I think there are multiple contingent checks interesting to bypass to sanitize a L2 pre-signed transactions :
CheckFinalTxAtTip() L731 in validation.cpp)CheckFinalTxAtTip() L731 in validation.ccp)CheckFeeRate() L858 in validation.cpp)CheckSequence() L588 in interpreter.cpp)CheckLocktTime() L554 in interpreter.cpp)Do we have more contingent checks interesting to bypass for a L2 ? The ones listed should be good for LN at least.
Instead of bypassing purely the timelocks checks, it could be interesting to allow passing MTP and height parameters, as suggested in #21413. That would enable to verify that your pre-signed transactions nLocktime and CLTV argument have been paired with correct values.
A further interesting feature would be to allow dummy signatures in the witness script. The intended use-case of such bypass options is to verify the consensus and policy validity of a transaction as pre-signed by your counterparty. However, to limit interference with signers policy it would be ideal for the local L2 node to not have to produce her signature during the verification flow.
Even further, on the LDK-side, we're targeting mostly mobile hosts, on which we might not be able to ship a Bitcoin Core node and access a RPC interface due to constraining OS application policies. It would be ideal to have a libstandardness in the spirit of the bitcoinconsensus library, exposing a high-level verify_transaction_standardness().
Anyway, good to go move step by step. Effectively it would be nice to break each option in its own PR. For passing a specific height, we could have testmempoolaccept(height=100).
@ariard If I understand correctly, are you suggesting the parameter schemes below?
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
{
{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{...},"..."},
{"bypass_relative_timelocks", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_absolute_timelocks", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_feerate_accuracy", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_csv_execution", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_cltv_execution", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
},
"\"options\""
},
or
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
{
{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{...},"..."},
{"mock_mtp", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"mock_height", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_feerate_accuracy", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_csv_execution", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
{"bypass_cltv_execution", RPCArg::Type::BOOL, RPCArg::Default{false}, "..."},
},
"\"options\""
},
`maxfeerate` becomes a member of an "options" object rather than
a positional argument.
The idea is that any new parameters in the future will also go into
options.
IMO the idea for mock_{mtp,height} is a better approach. "Valid in 9999 years" seems closer to "never valid" than "valid eventually".
I pushed some changes:
. struct MempoolTestCriteria was created to allocate all parameters suggested in #25434#pullrequestreview-1020896809. (https://github.com/bitcoin/bitcoin/pull/25434/commits/be52f8ace2e30aa5981852fafc4fe8f10eb8e759)
. bypass_timelocks has been split into bypass_relative_timelocks and bypass_absolute_timelocks, as suggested in the same comment. (https://github.com/bitcoin/bitcoin/pull/25434/commits/fccce1ef49d14ae54202583f1df39078398857c5)
. The tests were split in two commit: one tests that bypass_timelocls works and the other tests that it doesn't affect OP_CSV and OP_CLTV checks. (https://github.com/bitcoin/bitcoin/pull/25434/commits/d8e4a0918ed82cb3d7331590e736c01b733137d8 and https://github.com/bitcoin/bitcoin/pull/25434/commits/52a2e282568d404c9b48ab88e6ccd18ad140ad16).
102 | + {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", 103 | + { 104 | + {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, 105 | + "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"}, 106 | + {"bypass_absolute_timelocks", RPCArg::Type::BOOL, RPCArg::Default{false}, "Don't enforce nLocktime.\n"}, 107 | + {"bypass_relative_timelocks", RPCArg::Type::BOOL, RPCArg::Default{false}, "Don't enforce BIP68 relative lock-time.\n"}
Could you precise "Do not consensus-enforce BIP68" to make it clear it's a consensus check not policy one?
167 | + {"bypass_relative_timelocks", UniValueType(UniValue::VBOOL)}, 168 | + }, 169 | + true, true); 170 | + 171 | + if (options.exists("maxfeerate")) { 172 | + max_raw_tx_fee_rate = CFeeRate(AmountFromValue(options["maxfeerate"]));
if you like ternary I think you can do ternary here.
444 | @@ -444,6 +445,11 @@ class MemPoolAccept 445 | */ 446 | std::vector<COutPoint>& m_coins_to_uncache; 447 | const bool m_test_accept; 448 | + /* 449 | + * It represents the criteria that will be used to test the transactions 450 | + * without submitting them. 451 | + */ 452 | + const MempoolTestCriteria m_test_criteria;
i think the set of criterias, you might have multiple ones with the proposed API?
Changed it to const MemPoolBypass m_mempool_bypass in https://github.com/bitcoin/bitcoin/pull/25577/commits/26ee1a47e4bafe8faf60e16c987504d7d1b71c29
1053 | @@ -1041,6 +1054,9 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) 1054 | { 1055 | AssertLockHeld(cs_main); 1056 | AssertLockHeld(m_pool.cs); 1057 | + 1058 | + assert(args.m_test_criteria.AllowFinalizeTransaction());
As it's a belt-and-suspender to detect we don't have non-fully consensus-enforced transactions slipping in the mempool, i would rather go for something more explicit EnsureFullyValidatedTransaction() or EnsureNoBypassApplied().
1419 | @@ -1404,16 +1420,21 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, 1420 | } // anon namespace 1421 | 1422 | MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, const CTransactionRef& tx, 1423 | - int64_t accept_time, bool bypass_limits, bool test_accept) 1424 | + int64_t accept_time, bool bypass_limits, bool test_accept, 1425 | + const std::optional<MempoolTestCriteria>& test_criteria)
I wonder if it's not the good time to factor in a single new structure MemPoolCriteria encompassing bypass_limits, test_accept and test_criteria. The ctor could have an assert to ensure the client-provided bypass options such as timelocks are coming from testmempoolaccept.
229 | @@ -230,6 +230,15 @@ struct PackageMempoolAcceptResult 230 | : m_tx_results{ {wtxid, result} } {} 231 | }; 232 | 233 | +struct MempoolTestCriteria {
I think this structure could be documented. Though see comment above if it makes sense to factor in a single struct MemPoolBypass.
Sounds to move in the good direction. I think it could be good to have a "how-to" in doc/policy. I can write it if you would like.
I think it could be good to get one more ACK on the API by someone likely interested by such testmempoolaccept options cc @darosior @instagibbs
This is for test_accepts only, and not allowed in an actual submission
to mempool - see assert statements.
Provide an option to bypass BIP68 nSequence and nLockTime checks. This
means clients can use testmempoolaccept to check whether L2 transaction
chains (which typically have timelock conditions) are valid without
submitting them. Note that BIP112 and BIP65 are still checked since they
are script (non-contextual) checks. This does not invalidate any
signature or script caching.
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: glozow <gloriajzhao@gmail.com>
Test the bypass_timelock options in testmempoolaccept. This lets us
bypass BIP68 relative locktime checks (in nSequence) and absolute
locktime checks (in nLocktime).
Co-authored-by: glozow <gloriajzhao@gmail.com>
OP_CSV and OP_CLTV script checks are still done,
so setting bypass_timelocks=True doesn't mean that
bad scripts pass.
Co-authored-by: glozow <gloriajzhao@gmail.com>
@ariard I addressed most of your suggestions in a new PR (https://github.com/bitcoin/bitcoin/pull/25577).
That PR does not change behavior, it just refactors the use of test_accept and bypass_limits, allocating them in a new struct MemPoolBypass.
I rebased this PR on top of #25577.
I would like to see a stronger rationale for this change, as well as for the end goal of the path to conditionally bypass mempool checks. In short, i'm worried we are going to introduce a lot of complexity to this critical part of the codebase with only marginal -if any- benefits.
It seems like the motivation is helping to test applications which use offchain contracts. Any reasonable such application has a functional test framework (similar to ours) in which many sequences of events are tested in a blackbox manner. It is necessary to test the different spending paths of a Script, but does a lot more. Adding the option to bypass timelocks would only allow the most basic of those tests to use testmempoolaccept instead of generating a couple of blocks before using sendrawtransaction. Sure, that would reduce the runtime of those tests (by what, a few hundred milliseconds?). But i don't think it's a good tradeoff for us to make.
Furthermore, functional tests are for simulating real conditions and you necessary want to test the very same codepath that your application would use in prod. That is, using sendrawtransaction. We could assure them testmempoolaccept actually uses almost the very same codepath as sendrawtransaction (which is becoming less true as we add more complexity there), but i don't think it would be very convincing: if testing the exact codepath your application will trigger in prod is a node.generate(6) away, you'll just do it.
If the motivation for these PRs, and the direction things are taking, is "it'll help L2s apps to test better, maybe, eventually" i would suggest we take a step back and examine: how would such an application testing framework actually use it? For instance, here are two functional tests from two candidate projects i'm familiar with:
Both (implicitly or not) test the validity of transactions critical to the security of the protocol. I don't think testmempoolaccept with any form of bypass would help here. Neither would it for a lot other test.
Even assuming this feature might eventually be used. In #20833 we introduced testmempoolaccept, which could be used to quickly sanity check the "static" standardness bounds of a transaction. It was probably a good tradeoff. Now, we want to push this forward with allowing to also be able to test timelocked transactions. And by the same logic it was suggested to then allow for mocking the transaction with dummy values for Script. Where do we stop? We have a clear layer violation here. What is suggested with dummy signatures is precisely what we do internally with the DummySignatureChecker! Does it mean we should expose more of our internals via RPC? I don't think so, it would only result in cluttered code and interfaces and would not eliminate the need for any L2 application to have a functional test framework in which they create keys, spin up nodes, sign transactions and broadcast them in different sequences.
Therefore i think we need some kind of a plan for this bypass-everything direction, as well as some more motivation for going this way.
Motivation of this work is to provide tools enabling L2 applications to verify that issued transactions (not only pre-signed ones) can broadcast well on the p2p network, at least on the majority of nodes running the default Core policy. As a reminder, for this type of application, confirming transactions in due time is critical for the security of funds, a point I believe none of us deny.
For a typical Lightning implementation, this "standardness defect" risk can manifest itself at reception of the counterparty signatures for commitment / HTLC transactions. For the latest one, the nLocktime is set and the feerate is 0 (post option_anchors_zero_fee_htlc_tx). Therefore those transactions, of which the broadcast time is unknown, would be rejected by testmempoolaccept due to to non-compliance with contingent policy checks like timelocks or feerates.
As of today, if you would like to currently assert the standardness of those transactions (to eliminate vulnerabilities like CVE-2020-26895 to slip in), I think the solution is to rewrite by yourself all the policy checks in your implementation library (e.g in rust-bitcoin, we have a dust threshold) check. However, there is no guarantee that those checks are re-implemented correctly or even that there are exhaustive compared to Bitcoin Core mempool policy ones.
That testmempoolaccept code path diverges from sendrawtransaction is an unfortunate fact that iirc I raised during the reviews of the mempool package accept PRs. As sketched out above, I think the ideal interface would rather be alibstandardness with bypass options allowing direct access to AcceptToMemoryPool() not only to make the verification codepath as similar to the broadcast codepath, but also as an architecture convinency for mobile clients and a performance improvement for LN routing hops (relying on RPC call adds latency in HTLC relay due to process context-switch).
I don't think the advocated layer violation matters. In system engineering, you'll always find layer violation, e.g using ASM snippets in your high-level language source code to access CPU registers and boost your computation performance likewise with mathematic libraries (even if 99,99% of the userspace applications won't do that). So I think the question is more about if the layer violation is justified in the present case. About the scope, I think we should allow to check for as much standardness bounds that it makes sense for the application semantic.
That said, I think we should put apart the question of what should be the ideal interface to verify the standardness bounds (i.e RPC call, new shared library, new Capn' Proto interface, etc). In my understanding, I think the question where we diverge is if every L2 implementation should rewrite the Core standardness logic to verify its transaction validity, as much as you can in function of the application semantics, or rather we should expose this standardness logic in a straightforward way.
I think the first approach is not only duplicating the engineering cost for any L2 implementation, instead to factor in Core but also far more error-prone.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Based on #25434 (comment) and the fact that #21413 was in conceptual review stage, marking this as "needs conceptual review" until there is stronger support for this change.
Seeing #25532 closed, still renewing my interest to have an exportable Core "compliant" policy rules checker for second-layers toolchains. From an offline discussion with @darosior I think we observed there is a scale of severity difference between two class of L2 time-sensitive protocols:
update_add_htlc to add a HTLC output on both local/remote commitment transactions)From my comprehension, the second set of protocols is expected to increase in scope with the deployment of the interactive transaction construction protocol in the LN space, where a set of N participants might contribute to a transaction.
Looking forward to tackle this project by myself if there is no more interest by the current PR author after done with full-rbf, or at least introduce the subject at IRC meetings to collect more people thoughts on the design space and worthiness of such new API.
Looking forward to tackle this project by myself if there is no more interest by the current PR author after done with full-rbf, or at least introduce the subject at IRC meetings to collect more people thoughts on the design space and worthiness of such new API. @ariard thanks for the proactivity, I agree the concept should be discussed with more people (hence "needs conceptual review") - pinging @w0xlt to see if there is a plan to do something like this?
Btw just to clarify, #25532 was closed because it was blocked by this PR and this PR is currently also blocked. I wasn't rejecting the concept, just attempting to focus review attention more to make progress.
@glozow I'm not sure I understand your question correctly, but I'm available to discuss this PR (or any other changes).
From my perspective, this isn't / #21413 wasn't blocked based on code getting written, but because there isn't strong consensus that it's a good idea. I'm wondering what the plan is to get more conceptual review, e.g. through soliciting opinions from specific people or bringing it up in a meeting/mailing list. My question was whether you plan on doing the advocacy work, as it seems @ariard is offering to do so. Hope this clarifies what I meant.
I'm wondering what the plan is to get more conceptual review, e.g. through soliciting opinions from specific people or bringing it up in a meeting/mailing list.
Still thinking it would be nice to explain all the standardness malleability issues raised by policy for second-layers in a post, though ideally with PoC code. In the light of the recent LND bug due to old policy check in their dependency, I think well-engineered API/RPC calls would minimize the odds of such critical bugs in the future (orthogonal of how a watchtower infrastructure or better error-handling could have prevented this specific LND bug).
Happy to extend on the subject soon, though from my understanding the pipeline of "mempool-as-L2-interface" is quite busy those days between full-rbf, v3 policy, package RBF, ephemeral anchor and else. If someone would like to needle forward on that please do so (though a lot of conceptual things here).
<!--13523179cfe9479db18ec6c5d236f789-->
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
Closing for now. Not clear what the status of this is. Has needed rebase for > 6 months and had been rebased on top of a PR that is now closed.
