w0xlt
commented at 4:17 am on July 3, 2022:
contributor
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#pullrequestreview-1020896809.
Since this PR is built on top of #25577 and #25434, only the last 4 commits are new here.
mempool: change `maxfeerate` to be a members of an `options` argument
`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.
199b3148c0
mempool: make `test_accept` parameter explicit563dd147df
DrahtBot added the label
Mempool
on Jul 3, 2022
w0xlt force-pushed
on Jul 3, 2022
DrahtBot
commented at 3:40 pm on July 3, 2022:
contributor
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
commented at 1:38 am on July 6, 2022:
member
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
commented at 3:43 pm on July 6, 2022:
member
Could you (or perhaps @ariard, seeing the suggestion at #25434#pullrequestreview-1020896809) provide a use case for bypassing the minimum feerate checks?
ariard
commented at 1:52 am on July 8, 2022:
member
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
commented at 2:02 am on July 8, 2022:
member
@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
commented at 10:50 am on July 9, 2022:
contributor
@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.
validation: Add `MemPoolBypass` for bypassing mempool checks9e8703ca5f
mempool: add `MemPoolBypass` parameter to `ProcessNewPackage`14eff472b3
mempool: add `MemPoolBypass` parameter to `AcceptToMemoryPool`420a3c4dc0
mempool: add `MemPoolBypass` parameter to `ProcessTransaction`17db819b72
mempool: add `MemPoolBypass` parameter to `ATMPArgs`26ee1a47e4
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.
Co-authored-by: glozow <gloriajzhao@gmail.com>
b70a926081
rpc: add optional bypass_timelocks for testmempoolaccept
Co-authored-by: glozow <gloriajzhao@gmail.com>
afc299806c
test: check if `bypass_{absolute,relative}_timelock` works
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>
7ec6416310
test: check if bypass_timelocks do not affect scripts
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>
a006b8fd5a
ariard
commented at 0:59 am on July 14, 2022:
member
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.
validation: Add `bypass_feerate_accuracy` to `MempoolTestCriteria`20d049dd2e
luke-jr
commented at 7:34 pm on July 15, 2022:
member
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
commented at 3:52 pm on July 18, 2022:
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”.
DrahtBot added the label
Needs rebase
on Jul 18, 2022
glozow
commented at 10:28 am on August 1, 2022:
member
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
commented at 2:53 pm on October 3, 2022:
member
Closing for now as this builds on #25434 which still needs conceptual review. Can reopen when this is no longer blocked.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me