net: Remove forcerelay of rejected txs #17985
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Member
Does that mean #17303 is redundant? |
Member
Author
Member
Author
|
A test for forcerelay has been merged here 3b69310. The test should still pass after the changes here. |
Member
|
code review ACK fab08e0caa029aaaa683ad5a17b23f2dc0d66c56 |
Contributor
|
ACK fab08e0caa029aaaa683ad5a17b23f2dc0d66c56 Might be good to keep a |
fab08e0 to
fa9feaa
Compare
Member
Author
|
Thanks for the review. Added the logprint back. Also, I've rebased this to be able to extend the existing test case |
fa9feaa to
facb715
Compare
Member
Author
|
Force pushed to fixup the documentation |
Member
|
Concept ACK |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 26, 2020
facb715 net: Remove forcerelay of rejected txs (MarcoFalke) Pull request description: This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons: * While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See https://github.com/bitcoin/bitcoin/blob/4a072330763b3ff2d1b5c5b8d30a9517873ac6de/src/net_processing.cpp#L3862-L3866 * Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`) The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574 This feature was (intentionally or accidentally) removed in 4d8993b, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code. ACKs for top commit: hebasto: ACK facb715, locally running the unit and functional tests. Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
facb715 net: Remove forcerelay of rejected txs (MarcoFalke) Pull request description: This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons: * While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See https://github.com/bitcoin/bitcoin/blob/4a072330763b3ff2d1b5c5b8d30a9517873ac6de/src/net_processing.cpp#L3862-L3866 * Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`) The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574 This feature was (intentionally or accidentally) removed in 4d8993b, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code. ACKs for top commit: hebasto: ACK facb715, locally running the unit and functional tests. Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Aug 30, 2021
Summary: > This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons: > - While RelayTransaction enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. > - Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (mapRelay) > > This feature was (intentionally or accidentally) removed in 4d8993b, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code. This is a backport of [[bitcoin/bitcoin#17985 | core#17985]] Some comments were already updated on our side via D9325 and D9042. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9831
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
While
RelayTransactionenqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. Seebitcoin/src/net_processing.cpp
Lines 3862 to 3866 in 4a07233
Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (
mapRelay)The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.