8000 mempool: Add the `bypass_feerate_accuracy` option to `testmempoolaccept` by w0xlt · Pull Request #25532 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

mempool: Add the bypass_feerate_accuracy option to testmempoolaccept #25532

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

Closed
wants to merge 16 commits into from

Conversation

w0xlt
Copy link
Contributor
@w0xlt w0xlt commented Jul 3, 2022

This PR adds the the bypass_feerate_accuracy option to testmempoolaccept RPC, which allows skipping the minimum mempool fee rate validation.

This enables testing of HTLC-timeout transactions signed with 0-feerate (with BOLT9 option_anchors_zero_fee_htlc_tx).

For more context, check #25434 (review).

Since this PR is built on top of #25577 and #25434, only the last 4 commits are new here.

w0xlt added 2 commits July 1, 2022 15:48
`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.
@DrahtBot
Copy link
Contributor
DrahtBot commented Jul 3, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25577 (mempool, refactor: add MemPoolBypass by w0xlt)
  • #25487 ([kernel 3b/n] Decouple {Dump,Load}Mempool from ArgsManager by dongcarl)
  • #24007 ([mempool] allow tx replacement by smaller witness by LarryRuane)
  • #23897 (refactor: Move calculation logic out from 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.

@luke-jr
Copy link
Member
luke-jr commented Jul 6, 2022

Approach NACK.

Adding "bypass_" after bypass option is just a clunky way of rewriting #7533 / #20753.

For #25434 there might be a reason to handle it specially as a specific mock time/height, but that doesn't apply here.

@glozow
Copy link
Member
glozow commented Jul 6, 2022

Could you (or perhaps @ariard, seeing the suggestion at #25434 (review)) provide a use case for bypassing the minimum feerate checks?

@ariard
Copy link
ariard commented Jul 8, 2022

Let's say your a LN node receiving a counterparty's signature for a HTLC-timeout (BOLT2's commitment_signed). In the optimistic case, this transaction is never going to be broadcast as LN channels don't have a bounded lifetime. This HTLC-timeout is signed with 0-feerate (with option_anchors_zero_fee_htlc_tx) and should be aggregated with a fee-bumping input thanks to SIGHASH_SINGLE | SIGHASH_ANYONECANPAY. As the LN node, you would like to know if this transaction respects all the non-final and non-fee checks to broadcast it at anytime in the future, thus avoiding to accept a non-sane input from your counterparty.

@luke-jr are you raising that #7533 should be opened again as a technical approach is more straighforward ? If so I invite you to do so and let contributors evaluate which approach is the better. Or could you qualify more your concerns e.g the maintenance cost or API misusage ?

If you're raising the concern that Lightning shouldn't make safety-critical assumptions on transaction-relay policy, I'll let the burden on you to propose the design of a second-layer to scale bitcoin payments as much decentralized and privacy-preserving (once we have fix all the issues).

@luke-jr
Copy link
Member
luke-jr commented Jul 8, 2022

@luke-jr are you raising that #7533 should be opened again as a technical approach is more straighforward ? If so I invite you to do so and let contributors evaluate which approach is the better. Or could you qualify more your concerns e.g the maintenance cost or API misusage ?

@MarcoFalke has already reopened it as #20753.

If you're raising the concern that Lightning shouldn't make safety-critical assumptions on transaction-relay policy, I'll let the burden on you to propose the design of a second-layer to scale bitcoin payments as much decentralized and privacy-preserving (once we have fix all the issues).

It shouldn't, but that's beside the point I am making here. There's nothing inherent in the original Lightning design that necessarily makes such assumptions...

@jonatack
Copy link
Member
jonatack commented Jul 9, 2022

@w0xlt in the PR description, for reviewers I would suggest (a) mentioning that only the last 3 commits are new here, and (b) adding a "why" motivation in the PR description, because if the why doesn't make sense then the what probably doesn't matter.

w0xlt and others added 10 commits July 9, 2022 20:38
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
Copy link
ariard commented Jul 14, 2022

@MarcoFalke has already reopened it as #20753.

AFAICT #20753 only allows to bypass IsStandardTx / AreInputsStandard / IsWitnessStandard, not the feerate checks object of this PR>. Further, I think an API where the check is explicit rather than relying on error strings is more robust.

It shouldn't, but that's beside the point I am making here. There's nothing inherent in the original Lightning design that necessarily makes such assumptions...

And on that point, I really think the original Lightning design should have underscore more that failure to propagate a time-sensitive transaction should be considered as a break of the security model. Of course, if you're thinking about an ideal world where every LN node is a miner that wouldn't be an issue. That is not the real world. Or if you're saying that a LN node shouldn't make expectations about confirming a transaction before a timelock expiration, I believe that's the foundation of the payment channel design you're questioning.

@w0xlt w0xlt force-pushed the add_bypass_feerate_accuracy branch from 972674c to 7368c07 Compare July 15, 2022 19:14
@w0xlt
Copy link
Contributor Author
w0xlt commented Jul 15, 2022

Rebased on top of a006b8f.

@jonatack I added the suggested information in the PR description.

@luke-jr
Copy link
Member
luke-jr commented Jul 15, 2022

Further, I think an API where the check is explicit rather than relying on error strings is more robust.

Using a different JSON string for no reason is not an improvement.

@DrahtBot
Copy link
Contributor

🐙 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".

@glozow
Copy link
Member
glozow commented Aug 1, 2022

This HTLC-timeout is signed with 0-feerate (with option_anchors_zero_fee_htlc_tx) and should be aggregated with a fee-bumping input thanks to SIGHASH_SINGLE | SIGHASH_ANYONECANPAY. As the LN node, you would like to know if this transaction respects all the non-final and non-fee checks to broadcast it at anytime in the future

Seems like there isn't a reason to disable the feerate checks altogether. Rather, the better approach is to allow CPFP in the package submitted for testing, and the fee-bump should be included.

@glozow
Copy link
Member
glozow commented Oct 3, 2022

Closing for now as this builds on #25434 which still needs conceptual review. Can reopen when this is no longer blocked.

@glozow glozow closed this Oct 3, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0