net: Remove forcerelay of rejected txs
ClosedPublic
Actions

Authored by PiRK on Jul 22 2021, 14:33.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC1ee3e4bda416: net: Remove forcerelay of rejected txs

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 core#17985

Some comments were already updated on our side via D9325 and D9042.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jul 22 2021, 14:33

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 22 2021, 14:33

PiRK requested review of this revision.Jul 22 2021, 14:33

teamcity edited the summary of this revision. (Show Details)Jul 22 2021, 14:33

PiRK edited the summary of this revision. (Show Details)Jul 22 2021, 14:35

Harbormaster completed remote builds in B16246: Diff 29296.Jul 22 2021, 14:47

PiRK added a child revision: D9832: doc: Use precise permission flags where possible.Jul 22 2021, 15:09

Please backport 17984 in its own diff, it's difficult to make sure that nothing is missing here

src/net_processing.cpp
3672 ↗	(On Diff #29296)	layout

This revision now requires changes to proceed.Jul 23 2021, 06:46

needs backporting 17984

I managed to backport core#17984, but so far I'm struggling to add this new test. The challenge is to build a transaction that is invalid enough to not be added to the mempool, but not so invalid that it gets node0 banned for sending it.
The Core test seems to use some form of RBF feature that we disabled in rABC5b6419234d53 for Bitcoin ABC. For us, the transaction triggers txn-mempool-conflict

rebase onto D9906 (PR17984)

PiRK planned changes to this revision.Aug 23 2021, 16:20

PiRK edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B16372: Diff 29477.Aug 23 2021, 16:26

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: p2p_permissions.py ======

------- Stdout: -------
2021-08-23T16:25:30.912000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210823_162433/p2p_permissions_45
2021-08-23T16:25:32.964000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 127, in main
    self.run_test()
  File "/work/test/functional/p2p_permissions.py", line 32, in run_test
    self.check_tx_relay()
  File "/work/test/functional/p2p_permissions.py", line 149, in check_tx_relay
    self.nodes[0].signrawtransactionwithwallet(raw_tx)['hex'])
  File "/work/test/functional/test_framework/coverage.py", line 48, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 159, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Method not found (-32601)
2021-08-23T16:25:33.015000Z TestFramework (INFO): Stopping nodes
2021-08-23T16:25:33.271000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210823_162433/p2p_permissions_45
2021-08-23T16:25:33.271000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210823_162433/p2p_permissions_45/test_framework.log
2021-08-23T16:25:33.271000Z TestFramework (ERROR): 
2021-08-23T16:25:33.272000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20210823_162433/p2p_permissions_45' to consolidate all logs
2021-08-23T16:25:33.272000Z TestFramework (ERROR): 
2021-08-23T16:25:33.272000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2021-08-23T16:25:33.272000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2021-08-23T16:25:33.272000Z TestFramework (ERROR):

Each failure log is accessible here:
Bitcoin ABC functional tests: p2p_permissions.py

rebase. The current master uses signrawtransactionwithkey rather than signrawtransactionwithwallet, so it should fix the build-without-wallet CI error.

Harbormaster completed remote builds in B16457: Diff 29632.Aug 30 2021, 07:54

Fabien accepted this revision.Aug 30 2021, 08:15

This revision is now accepted and ready to land.Aug 30 2021, 08:15

Closed by commit rABC1ee3e4bda416: net: Remove forcerelay of rejected txs (authored by MarcoFalke <falke.marco@gmail.com>, committed by PiRK). · Explain WhyAug 30 2021, 08:39

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC1ee3e4bda416: net: Remove forcerelay of rejected txs.