Skip to content

doc requirement that replacement must have higher feerate than direct conflicts #25382

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 16, 2022
Merged

doc requirement that replacement must have higher feerate than direct conflicts #25382

merged 1 commit into from
Jun 16, 2022

Conversation

glozow
Copy link
Member

@glozow glozow commented Jun 15, 2022 •
edited
Loading

RBF policy requires the replacement transaction have a higher feerate than each of the directly conflicting transactions (see PaysMoreThanConflicts).
It was pointed out that this rule is undocumented: #25038 (comment)

@glozow glozow mentioned this pull request Jun 15, 2022
Closed
sdaftuar
sdaftuar reviewed Jun 15, 2022
View reviewed changes
@laanwj laanwj added the Docs label Jun 15, 2022
@instagibbs
Copy link
Member

ACK on text, probably don't want duplicate tests though

@DrahtBot
Copy link
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:

  • #25353 (Add a -fullrbf node setting by ariard)
  • #22867 (test: Extend test coverage of BIP125 and document confusing/inconsistent behavior by mjdietzx)

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.

@DrahtBot DrahtBot mentioned this pull request Jun 15, 2022
Merged
@glozow glozow force-pushed the 2022-06-rbf-feerate-rule branch from dedfdda to 2224bca Compare June 15, 2022 19:24
w0xlt
w0xlt approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2224bca

@laanwj
Copy link
Member

laanwj commented Jun 15, 2022 •
edited
Loading

ACK 2224bca
Might want to update the PR title for removing the test.

@sdaftuar
Copy link
Member

ACK

darosior
darosior reviewed Jun 15, 2022
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2224bca

Should have catched it in #23711 :/

ariard
ariard approved these changes Jun 16, 2022
View reviewed changes
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2224bca

doc/policy/mempool-replacements.md
@@ -51,6 +51,13 @@ other consensus and policy rules, each of the following conditions are met:
significant portions of the node's mempool using replacements with multiple directly conflicting
transactions, each with large descendant sets.

6. The replacement transaction's feerate is greater than the feerates of all directly conflicting
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super-nit: strictly greater :)

I think some L2s stacks are duplicating a bunch of mempools standardness checks in their broadcast backend (at least LDK do so). Allowing to issue replacement transactions which are feerate valid against <= and you're Game Over (tm). So better than developers understand really well the mempool checks.

(Please don't update the PR for that, not necessary to invalidate previous ACKs)

@DrahtBot DrahtBot mentioned this pull request Jun 16, 2022
Closed
t-bast
t-bast approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2224bca

@fanquake fanquake changed the title doc and test requirement that replacement must have higher feerate than direct conflicts doc requirement that replacement must have higher feerate than direct conflicts Jun 16, 2022
@fanquake fanquake merged commit d683221 into bitcoin:master Jun 16, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 16, 2022
@fanquake
Merge bitcoin#25382: doc requirement that replacement must have highe…
457143b 
…r feerate than direct conflicts

2224bca [doc] RBF feerate rule (glozow)

Pull request description:

  RBF policy requires the replacement transaction have a higher feerate than each of the directly conflicting transactions (see `PaysMoreThanConflicts`).
  It was pointed out that this rule is undocumented: bitcoin#25038 (comment)

ACKs for top commit:
  laanwj:
    ACK 2224bca
  w0xlt:
    ACK bitcoin@2224bca
  darosior:
    ACK 2224bca
  ariard:
    ACK 2224bca
  t-bast:
    ACK bitcoin@2224bca

Tree-SHA512: 0d3915100973b66d115c3294f3037d0c5473c00236c8823a4b2fe12ff172457af56c295b41ac0ef983de030f40f0817c046bb486bf60a5a593d1c4524fe1b9d2
@fanquake fanquake added this to the 24.0 milestone Sep 15, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@instagibbs instagibbs Awaiting requested review from instagibbs

5 more reviewers

@sdaftuar sdaftuar sdaftuar left review comments

@darosior darosior darosior left review comments

@ariard ariard ariard approved these changes

@t-bast t-bast t-bast approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
10 participants
@glozow @instagibbs @DrahtBot @laanwj @sdaftuar @darosior @ariard @t-bast @w0xlt @fanquake