Add NODE_TXRELAY_V2. #30837
Add NODE_TXRELAY_V2. #30837
Conversation
This does not add default signaling of NODE_TXRELAY_V2 as node local services.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. |
| @@ -329,6 +329,9 @@ enum ServiceFlags : uint64_t { | |||
| // NODE_P2P_V2 means the node supports BIP324 transport | |||
| NODE_P2P_V2 = (1 << 11), | |||
|
|
|||
| // NODE_TXRELAY_V2 means the node supports BIPXXX transaction-relay v2 protocol | |||
| NODE_TXRELAY_V2 = (1 << 12), | |||
There was a problem hiding this comment.
(in case you're not waiting for concept acks)
/src/protocol.cpp:94:13: error: enumeration value 'NODE_TXRELAY_V2' not handled in switch [-Werror,-Wswitch]
94 | switch ((ServiceFlags)service_flag) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
will fix it, still looking more for reviews on the BIPs themselves for the disruption minimization approach of restraining the acceptance of unsolicited transactions among upgraded peers only.
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
🤔 There hasn't been much activity lately and the CI seems to be failing. If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in. |
|
Closing for now, due to inactivity for a few months, and failing CI, since it was opened. Please leave a comment, if you want this reopened. Alternatively, you can also open a new pull, since there hasn't been any discussion here anyway, except for discussions around the failing CI. |
|
@maflcko Thanks if this PR can be re-opened. The more ample rational behind this work has been published here:https://bitcoinops.org/en/newsletters/2024/12/06/ and it was under embargo for a bunch of months, as we were considering the impact on lightning implementations. I’ll go back on this PR + #30572 soon (tm). The problem is serious and for now it’s more conceptual review that is calling for than fixing a CI. |
|
If that's the case, it's best to reopen this only when there is (1) more conceptual support and (2) you've had the time to add tests and fix CI. As per the contributing guidelines, PRs are expected to have test coverage and not break existing tests. |
|
@glozow Thanks for the comment. Have a look on the original PR on #21224, when all the existing tests at the time were fixed, before a real conversation on the conceptual trade-offs that one can evaluate at high-level was effectively done. Sorry, not sorry, it’s not rational to fix CI if it’s to waste time again, before there is conceptual convergence. As a contributor yourself, before to be a maintainer, and I think someone able to conceptually review PRs yourself, if there are technical aspects of bitcoin P2P related to this PR that you don’t understand, feel free to ask questions, I’ll answer them. |
|
@ariard Asking for conceptual feedback is reasonable, but here I don't even see any concept being proposed, neither in the PR nor in the associated BIP draft. It is just introducing a new flag, without any specification regarding what should be changed in the new protocol, nor even any discussion about kinds of changes would be involved. |
|
@sipa Yeah now it’s post-embargo, I’ll update the 2 bips with more context, fix the code at minima for the 2 PRs, and prepare a list of questions on the trade-offs or comparison of different approaches, and then re-open a PR or ask on the ML for conceptual feedback. Should do it in the coming weeks. |
This is the top commit from #30572, where the new node service bit support commit is extracted on its own.
See the corresponding BIP draft for motivation: bitcoin/bips#1663
Dissociating this change from halting the processing of unrequested transaction, allow the node service bit support to be used for further policies and mechanisms, beyond this mechanism only.