Extract CConnman::RelayTransaction #15992

pull Empact wants to merge 2 commits into bitcoin:master from Empact:relay-inventory changing 7 files +52 −28

Empact commented at 4:27 PM on May 9, 2019: member
- Implement RelayTransaction on CConnman only, rather than chain interface, BroadcastTransaction, and locally in net processing.
- Split node PushInventory into PushBlockInventory and PushTransactionInventory, as at every call-site the block/tx distinction is already made.
Extracted from #15253
Empact renamed this:
~~Refactor~~
Extract RelayTransaction and PushBlock/TransactionInventory
on May 9, 2019
promag commented at 4:38 PM on May 9, 2019: member

utACK d48c4bf, nice and simple refactor.
DrahtBot added the label P2P on May 9, 2019
DrahtBot commented at 5:52 PM on May 9, 2019: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15945 (net: Log unknown CInv in PushInventory by Bushstar)
- #15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
- #15253 (Net: Consistently log outgoing INV messages by Empact)
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.
gmaxwell commented at 2:43 AM on May 10, 2019: contributor

I don't see what value this adds, particularly doing things like creating functions to encapsulate single lines of code.
Empact force-pushed on May 10, 2019
Empact commented at 4:38 AM on May 10, 2019: member

As noted I wrote this to enable the logging introduced here: https://github.com/bitcoin/bitcoin/commit/389709e02c165ef06db35986e1619c16824f880f

In terms of justifying it independently, in the first case, having a single implementation protects against unintentional divergence going forward (the DRY principle), also simplifies by removing a layer of indirection relative to CConnman.

~Dropped the second commit as I found no satisfying independent justification for it.~
Empact renamed this:
~~Extract RelayTransaction and PushBlock/TransactionInventory~~
Extract CConnman::RelayTransaction
on May 10, 2019
Empact commented at 5:29 AM on May 10, 2019: member

On second look, consider #15945. PushInventory currently silently ignores unrecognized INV types, which is a potential point of failure / confusion for ineffective callers. This narrows the Push*Inventory calls to those which can and will be handled. Restored the initial version.

in src/net.h:850 in d48c4bffc1 outdated

 848 | -        if (inv.type == MSG_TX) {
 849 | -            if (!filterInventoryKnown.contains(inv.hash)) {
 850 | -                setInventoryTxToSend.insert(inv.hash);
 851 | -            }
 852 | -        } else if (inv.type == MSG_BLOCK) {
 853 | -            vInventoryBlockToSend.push_back(inv.hash);

MarcoFalke commented at 3:18 PM on May 10, 2019:

Could keep the implementation inlined in the header to keep the diff smaller?

sdaftuar commented at 4:08 PM on May 11, 2019: member

IMO it makes more sense to refactor transaction/block relay logic out of net.cpp and into net_processing.cpp. So further consolidating this in connman seems like a small step in the wrong direction, and not worth the friction of changing around how this logic works.
Empact force-pushed on May 15, 2019
Empact force-pushed on May 15, 2019
Empact force-pushed on May 15, 2019

[moveonly] net: Extract RelayTransaction

Note the hashes passed more closely match the surrounding code.

174109efb6

[moveonly] net: Split CNode::PushInventory into Block and Transaction variants
Only 3 callers, and in each case they're explicit about which they are doing.
8da550d0b6
Empact force-pushed on May 16, 2019
Empact commented at 6:41 PM on May 16, 2019: member

@sdaftuar Not sure what you have in mind, but it's now in a separate file rather than on CConnman.
sdaftuar commented at 7:26 PM on May 16, 2019: member

IMO it doesn't seem like this accomplishes anything, and it conflicts with some other PRs, so I would tend to NACK.
MarcoFalke commented at 7:37 PM on May 16, 2019: member

Seems too controversial for a simple change such as this. Going to close this for now.
MarcoFalke closed this on May 16, 2019
Empact deleted the branch on May 17, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15253 Net: Consistently log outgoing INV messages

Extract CConnman::RelayTransaction #15992

Conflicts