Skip to content
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

doc: add release note for #27460 (new importmempool RPC) #28637

Merged

Conversation

theStack
Copy link
Contributor

This PR adds a missing release note for #27460.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 11, 2023

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK glozow
Stale ACK ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label Oct 11, 2023
@fanquake fanquake added this to the 26.0 milestone Oct 11, 2023
@theStack theStack force-pushed the 202310-doc-add_importmempool_release_note branch 2 times, most recently from 0b80201 to 356a752 Compare October 11, 2023 20:20
Copy link
Member

@ismaelsadeeq ismaelsadeeq left a comment

Choose a reason for hiding this comment

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

ACK 356a752

@ajtowns
Copy link
Contributor

ajtowns commented Oct 12, 2023

If this is going to be advertised in the release notes, shouldn't it include more warnings? eg, importing someone else's mempool.dat could result in pinning vectors so you miss out on txs being relayed across the regular network. If you enable any of the options when importing (fee deltas or unbroadcast set or time info) that could also cause problems.

@glozow
Copy link
Member

glozow commented Oct 12, 2023

If this is going to be advertised in the release notes, shouldn't it include more warnings? eg, importing someone else's mempool.dat could result in pinning vectors so you miss out on txs being relayed across the regular network. If you enable any of the options when importing (fee deltas or unbroadcast set or time info) that could also cause problems.

+1
Could also mention that, If you want to apply fee deltas, it is recommended to use the getprioritisedtransactions and prioritisetransaction RPCs instead of the apply_fee_delta_priority option to avoid duplicates.

@theStack theStack force-pushed the 202310-doc-add_importmempool_release_note branch 2 times, most recently from bebd25a to 5960364 Compare October 12, 2023 14:34
@theStack
Copy link
Contributor Author

@ajtowns: Thanks for the feedback. I added one generic warning about import untrusted files (taken from the RPC help) and @glozow's recommendation for applying fee deltas. Happy to take more suggestions.

@glozow
Copy link
Member

glozow commented Oct 13, 2023

LGTM, @ajtowns ?

@willcl-ark
Copy link
Member

Could also mention that, If you want to apply fee deltas, it is recommended to use the getprioritisedtransactions and prioritisetransaction RPCs instead of the apply_fee_delta_priority option to avoid duplicates.

What exactly does "to avoid duplicates" mean here? That we can end up with two versions of the same tx in the mempool (with different deltas), or something else?

@glozow
Copy link
Member

glozow commented Oct 13, 2023

Could also mention that, If you want to apply fee deltas, it is recommended to use the getprioritisedtransactions and prioritisetransaction RPCs instead of the apply_fee_delta_priority option to avoid duplicates.

What exactly does "to avoid duplicates" mean here? That we can end up with two versions of the same tx in the mempool (with different deltas), or something else?

No you'll never have duplicates in your mempool. I mean it just blindly applies the deltas, so you might prioritize a transaction twice. Though I guess using the other RPC doesn't inherently prevent this; it's just a way for you to sanitize what the deltas are + check what you already have before applying them. Sentence could be dropped as it seems like it could be confusing.

@ajtowns
Copy link
Contributor

ajtowns commented Oct 13, 2023

LGTM, @ajtowns ?

Seems okay. Perhaps "over-prioritising" or "double counting" would work better than "duplicates".

@willcl-ark
Copy link
Member

LGTM, @ajtowns ?

Seems okay. Perhaps "over-prioritising" or "double counting" would work better than "duplicates".

Agree. I was thinking something even more explicit though, like this:

"If you want to apply fee deltas, it is recommended to use the getprioritisedtransactions and prioritisetransaction RPCs instead of the apply_fee_delta_priority option to avoid double-prioritising any already-prioritised transactions in the mempool.

Co-authored-by: glozow <gloriajzhao@gmail.com>
@theStack theStack force-pushed the 202310-doc-add_importmempool_release_note branch from 5960364 to 1b672eb Compare October 15, 2023 16:16
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

ACK 1b672eb

@glozow glozow merged commit 1803fee into bitcoin:master Oct 18, 2023
16 checks passed
@theStack theStack deleted the 202310-doc-add_importmempool_release_note branch October 18, 2023 11:11
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023
…portmempool` RPC)

1b672eb doc: add release note for bitcoin#27460 (new `importmempool` RPC) (Sebastian Falbesoner)

Pull request description:

  This PR adds a missing release note for bitcoin#27460.

ACKs for top commit:
  glozow:
    ACK 1b672eb

Tree-SHA512: 89deadbfd6779e6eb19801c9fe7459a9876b920d44e09df102774c1eb8b3c0716462613dc99d1711eda4bd959ea61595b33f4528424ac02cf1af6cb4e5f1f0e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants