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

glozow · 2021-03-10T23:31:25Z

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.

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.

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

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.

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

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.

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.

jnewbery · 2021-03-10T23:44:15Z

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

DrahtBot · 2021-03-11T04:17:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: remove Optional & nullopt #21415 (refactor: remove Optional & nullopt by fanquake)
[Bundle 5/n] Prune g_chainman usage in RPC modules #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.

ariard

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.

ariard · 2021-03-15T17:23:07Z

src/validation.cpp

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


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).

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.

ariard · 2021-03-15T17:24:05Z

src/rpc/rawtransaction.cpp

@@ -889,6 +889,7 @@ static RPCHelpMan testmempoolaccept()
                        },
                        },
                    {"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"},
+                    {"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"}


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

Prefer turning maxfeerate into an options Object

harding · 2021-04-27T00:40:09Z

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 · 2021-04-28T19:56:23Z

@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).

DrahtBot · 2021-12-13T23:18:41Z

🐙 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 · 2022-01-10T01:35:16Z

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

ariard · 2022-06-16T19:51:19Z

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 · 2022-06-18T15:06:06Z

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 :)

glozow added 9 commits March 8, 2021 10:12

doc/style followups in MempoolAcceptResult

2dc0235

[validation] make CheckSequenceLocks accept coinsview

ce6a256

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.

[validation] add CoinsViewTemporary for mempool validation

6bc9e81

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

[test] functional test for packages in RPCs

06f8fce

[rpc] add optional bypass_timelocks for testmempoolaccept

9270093

fanquake added the Needs Conceptual Review label Mar 10, 2021

This was referenced Mar 11, 2021

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

refactor: remove Optional & nullopt #21415

Merged

ariard reviewed Mar 15, 2021

View reviewed changes

Modusto25 approved these changes May 16, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 13, 2021

glozow added the Up for grabs label Jun 16, 2022

glozow closed this Jun 16, 2022

w0xlt mentioned this pull request Jun 21, 2022

mempool: Add option to bypass contextual timelocks in testmempoolaccept #25434

Closed

maflcko removed the Up for grabs label Jun 21, 2022

bitcoin locked and limited conversation to collaborators Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glozow commented Mar 10, 2021

jnewbery commented Mar 10, 2021

DrahtBot commented Mar 11, 2021 •

edited

ariard left a comment

ariard Mar 15, 2021

glozow Mar 23, 2021

ariard Mar 15, 2021

luke-jr Jun 24, 2021

harding commented Apr 27, 2021 •

edited

glozow commented Apr 28, 2021

DrahtBot commented Dec 13, 2021

instagibbs commented Jan 10, 2022

ariard commented Jun 16, 2022 •

edited

glozow commented Jun 18, 2022

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

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

Conversation

glozow commented Mar 10, 2021

jnewbery commented Mar 10, 2021

DrahtBot commented Mar 11, 2021 • edited

Conflicts

ariard left a comment

Choose a reason for hiding this comment

ariard Mar 15, 2021

Choose a reason for hiding this comment

glozow Mar 23, 2021

Choose a reason for hiding this comment

ariard Mar 15, 2021

Choose a reason for hiding this comment

luke-jr Jun 24, 2021

Choose a reason for hiding this comment

harding commented Apr 27, 2021 • edited

glozow commented Apr 28, 2021

DrahtBot commented Dec 13, 2021

instagibbs commented Jan 10, 2022

ariard commented Jun 16, 2022 • edited

glozow commented Jun 18, 2022

DrahtBot commented Mar 11, 2021 •

edited

harding commented Apr 27, 2021 •

edited

ariard commented Jun 16, 2022 •

edited