[rfc] add option to bypass contextual timelocks in testmempoolaccept?

glozow commented at 11:31 pm on March 10, 2021: member

Draft because this depends on an open PR and I’m only looking for Concept ACKs for now.

With #20833, we can test transactions chains against validation rules + policy without broadcasting them. There was discussion about how to make it useful by applications which have spending paths that require some delay (e.g. “if Alice doesn’t redeem, Bob can get the coins 144 blocks later”). This PR implements a test_accept-only flag, bypass_timelocks, to skip CheckFinalTx and CheckSequenceLocks, i.e. that tx nLocktime and nSequence are ok based on the current block height/time. Script checks are still done as-is, so if the transaction itself has an error beyond not being final yet, it still fails.

I’ve added a few lines here and there in the functional tests just to make sure it works as expected. If people think this is useful, I can write more tests from scratch.

doc/style followups in MempoolAcceptResult 2dc0235afe

[validation] make CheckSequenceLocks accept coinsview

Allows CheckSequenceLocks to use heights and coins from any CoinsView
provided. The typical usage would still be to create a CCoinsViewMemPool
from a pool and pass it in.

ce6a2566f9

[validation] add CoinsViewTemporary for mempool validation

Functions in CCoinsViewCache need to be marked virtual so that
CCoinsViewTemporary can override them.

6bc9e810c7

[validation] package validation test_accept=true

Only allow test accepts for now. Use the CCoinsViewTemporary to keep
track of coins created by each transaction so that subsequent
transactions can spend them. Uncache all coins since we only
ever do test accepts (Note this is different from ATMP which doesn't
uncache for valid test_accepts) to minimize impact on the coins cache.

Require that the input txns have no conflicts and be ordered
topologically. In the future, we should sanitize and sort the package
before validation.

7d0e9415d8

[rpc] allow multiple txns in testmempoolaccept

Only allow "packages" with no conflicts, sorted in order of dependency,
and no more than -limitdescendantcount for now.

Note that these groups of transactions don't necessarily need to be
exactly 1 package or have any dependency relationships.

it may help to use -w to view the diff

1b90ac977f

[test] functional test for packages in RPCs 06f8fce092

[validation] add option to bypass contextual timelock checks

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.

74fe9c2831

[rpc] add optional bypass_timelocks for testmempoolaccept 92700936d0

[test] bypass_timelocks

Test the bypass_timelocks option in testmempoolaccept. This lets us
bypass BIP68 relative locktime checks (in nSequence) and absolute
locktime checks (in nLocktime).  OP_CSV and OP_CLTV script checks are
still done, so setting bypass_timelocks=True doesn't mean that bad
scripts pass.

9bdaa14de5

fanquake added the label Needs Conceptual Review on Mar 10, 2021

jnewbery commented at 11:44 pm on March 10, 2021: member

Concept ACK. This will probably be useful for people developing lightning and other offchain contracts that use timelocks/relative timelocks.

DrahtBot commented at 4:17 am on March 11, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21415 (refactor: remove Optional & nullopt by fanquake)
#21391 ([Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl)

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.

JeremyRubin added this to the "General Testing" column in a project

in src/validation.cpp:593 in 74fe9c2831 outdated

589@@ -590,6 +590,7 @@ class MemPoolAccept
590         const CChainParams& m_chainparams;
591         const int64_t m_accept_time;
592         const bool m_bypass_limits;
593+        const bool m_bypass_timelocks;

ariard commented at 5:23 pm on March 15, 2021:

I think I would get even further and have m_bypass_absolute_timelocks/m_bypass_relative_timelocks.

You might be willingly to shrug about the relative timelocks encumbering the nSequence of a transaction while being sure the nLocktime make sense (e.g spotting bug in your anti-fee snipping logic).

glozow commented at 2:13 pm on March 23, 2021:

How common is it to have both in a transaction? 😮 If it’s very common then sure. But also for this particular case, checking absolute but not relative, calling another testmempoolaccept without bypass_timelocks and checking that it fails on non-BIP68-final would do the trick, since nLockTime is checked before the nSequences.

in src/rpc/rawtransaction.cpp:892 in 92700936d0 outdated

888@@ -889,6 +889,7 @@ static RPCHelpMan testmempoolaccept()
889                         },
890                         },
891                     {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kB\n"},
892+                    {"bypass_timelocks", RPCArg::Type::BOOL, /* default */ "false", "Don't enforce BIP68 sequence locks and timelocks. Don't use unless you know what you're doing!\n"}

ariard commented at 5:24 pm on March 15, 2021:

I’ll write a doc/release note for this :)

luke-jr commented at 3:25 pm on June 24, 2021:

Prefer turning maxfeerate into an options Object

ariard commented at 5:25 pm on March 15, 2021: member

Concept ACK, this would be useful today to check some L2 flows (e.g a package of DLC funding transaction + dual-signed, timelocked refund transaction). Thanks for working on this.

harding commented at 0:40 am on April 27, 2021: contributor

Concept ACK. I hate to suggest significant scope creep, but I wonder if it might be more useful to:

Allow passing in MTP and height parameters, which will be used to evaluate all absolute timelocks and heightlocks, plus any relative time/heightlocks for confirmed UTXOs,
Separately, allow overriding relative timelocks between unconfirmed transactions in the package.

For contract protocol developers making heavy use of timelocks, I think this gets you the flexible standardness checker @ariard was requesting in #20833. Examples:

Alice gives Bob a transaction timelocked for 2021-12-31. Bob wants to know if he’ll be able to submit it on January 1st. testmempoolaccept(rawtxs=[tx], mtp="2022-01-01")
Alice has a recently confirmed UTXO locked with 123 OP_CSV. She gives Bob a spend of that UTXO and Bob wants to know if he’ll be able to submit it by height 800,000. testmempoolaccetp(rawtxs=[tx], height=800000)
Alice’s LN node offers Bob’s LN node a commitment transaction with a 1 OP_CSV child HTLC. Bob wants to make sure the commitment tx is standard and that his spend of the HTLC would also be standard if the commitment gets confirmed. testmempoolaccept(rawtxs=[commitment, htlc], bypass_relative_timelocks_from_inputs_created_in_this_package=true) (obviously, a better name is needed)

I think the takeaways here are that you can always easily check absolute timelocks and heightlocks by passing in later specific times and heights (and checking things when it’s easy is better than bypassing them). It’s also easy to check relative locks in the same way for spends of UTXOs that have been confirmed. It’s only for relative timelocks between unconfirmed transactions where I think you want to truly bypass checks.

I think the above also addresses @ariard’s comment about having more granular control over the bypasses.

glozow commented at 7:56 pm on April 28, 2021: member

@harding ah, that is super helpful! Thanks!

For overriding relative timelocks between unconfirmed transactions in a package, approach-wise, I think we could just add a special PACKAGE_HEIGHT value for coins added by package transactions (similar to the MEMPOOL_HEIGHT value).

Modusto25 approved

DrahtBot added the label Needs rebase on Dec 13, 2021

DrahtBot commented at 11:18 pm on December 13, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

instagibbs commented at 1:35 am on January 10, 2022: member

concept ACK, this type of feature has has the potential to help anyone using timelocks, a key feature in Bitcoin scripting and smart contracts.

not scope creep but wondering what this could look like for pre-image reveals, adapter sigs

glozow added the label Up for grabs on Jun 16, 2022

glozow closed this on Jun 16, 2022

ariard commented at 7:51 pm on June 16, 2022: member

Sad to see this closed. From the LDK-side, I think we’re still interested to integrate such enhanced testmempoolaccept in our release process to verify that all the transaction we issue are standard-compliant. Even further, if a follow-up work extracts the transaction mempool acceptance logic in a libstandardness, I think we’ll consider to include it directly at any reception of a counterparty’s transaction to avoid accidental frailties or exploitable vulnerabilities like CVE-2020-26895.

That said, I think I’m also to blame as I advocated this change without following-up on the review bandwidth. Maybe we can reconsider it, when we’re done with other policy-related PRs, like package relay :)

glozow commented at 3:06 pm on June 18, 2022: member

I wasn’t really working on this (needed rebase for a long time) so I mostly wanted to mark it up for grabs in case somebody else wants to work on it. I do plan to revisit in the future when I have more time :)

MarcoFalke removed the label Up for grabs on Jun 21, 2022

DrahtBot locked this on Jun 21, 2023

[rfc] add option to bypass contextual timelocks in testmempoolaccept? #21413

Conflicts