Add NODE_BITCOIN_CASH to required services for outbound
peer connections.
Not required for inbound peers.
Details
- Reviewers
deadalnix CCulianu - Group Reviewers
Restricted Project - Commits
- rSTAGINGd68e7e48547c: Peer preferentially with NODE_CASH nodes
rABCd68e7e48547c: Peer preferentially with NODE_CASH nodes
- Remove peers.dat
- Start up node
- Monitor peer connections using e.g. $ bitcoin-cli getpeerinfo | egrep '(service|subver|inbound)'
- Check that all peers for which inbound is false have bit 5 (0x20) set in their reported services
- Connect to various other types of peers (BU, Classic, XT) using addnode and check that connections are successful
- Check that inbound peers without NODE_CASH service bit set can connect (you should get occasional crawlers or Satoshi clients connecting with services < 0x20)
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- preferential_peering_on_nodecash
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 713 Build 713: arc lint + arc unit
Event Timeline
I've tested this on two mainnet nodes for a while now.
Seems no problem in keeping connections to existing nodes, and I have not noticed any non-NODE_CASH nodes in my outbounds anymore.
The relevant netcode is unfortunately not easily unit-testable or system-testable except for manual procedure as indicated in Test Plan.
I like the addition of the command-line argument. I can imagine use cases where someone might want this turned off explicitly (say, when synching up or whatnot). We should leave the command-line argument in.
I was reading over the code and I saw this in net.cpp line 1876:
// only connect to full nodes if ((addr.nServices & REQUIRED_SERVICES) != REQUIRED_SERVICES) { continue; }
So I thought that would mean -prefcashpeer=0 wouldn't work. But it does work. *scratches head*. Maybe I am misreading the code... Can you enlighten me on what's going on?
Anyway this works as intended...
UPDATE: Yeah the REQUIRED_SERVICES constant means even with -prefcashpeer=0, we won't relay or store addresses that aren't cash. Is this intended? (see net_processing.cpp line 1708).
Is this what we want for -prefcashpeer=0?
No, you're right, -noprefcashpeer is not working as intended. I'd need to elaborate the condition you pointed at.
If we want to keep the option, that is.
I like the option a lot, but I know deadalnix is against it. if you do keep the option the easiest thing to do really is take the info for required services from the config that gets passed down to net/connman/etc and get rid of the REQUIRED_SERVICES as a static global...
Not hard to fix, really.
I like the option, FWIW.
Prediction: deadalnix will shoot it down though.. and ask you to eliminate the option. In which case your work is much easier! :)
Remove the option to disable preferential cash node peering
After consideration, it is fair that we use NODE_CASH nodes
even during IBD (the main use for the option pretty much would
be to distribute IBD load for blocks prior to the UAHF fork).