doc: Use precise permission flags where possible

MarcoFalke commented at 3:50 pm on July 9, 2020: member

Instead of mentioning the all-encompassing -whitelist* settings, change the docs to mention the exact permission flag that will influence the behaviour.

This is needed because in the future, the too-broad -whitelist* settings (they either include all permission flags or apply to all peers) might be deprecated to require the permission flags to be enumerated.

Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the -whitelist* terminology is of no help.

MarcoFalke added the label Docs on Jul 9, 2020

jonatack approved

jonatack commented at 4:10 pm on July 9, 2020: member

ACK fa52c2c

laanwj commented at 5:21 pm on July 9, 2020: member

ACK fa52c2c90fec1971a83fae08fe5fcad0be9b8833

Sjors commented at 6:07 pm on July 9, 2020: member

ACK fa52c2c

Maybe also change these log messages:

Not relaying non-mempool transaction %s from whitelisted peer
Force relaying tx %s from whitelisted peer
Warning: not punishing whitelisted peer
Timeout downloading headers from whitelisted peer=%d, not disconnecting

And clarify code comment:

// Reset in-flight state in case of whitelist

MarcoFalke commented at 6:31 pm on July 9, 2020: member

// Reset in-flight state in case of whitelist

I have no idea what that comment means

jnewbery commented at 6:37 pm on July 9, 2020: member

Concept ACK

Sjors commented at 6:44 pm on July 9, 2020: member

// Reset in-flight state in case of whitelist

I have no idea what that comment means

Isn’t that totally obvious from its introduction by @TheBlueMatt in #8068? :-)

Found a comment:

We should clear the block as no longer being in flight. Otherwise we’re relying on the Misbehaving() to cause a disconnect, which would eventually clear the state – but this seems error prone, eg if the node happens to be whitelisted.

So I think it’s OK for “whitelisted” to be vague here.

DrahtBot commented at 2:26 am on July 10, 2020: 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:

#19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
#18044 (Use wtxid for transaction relay by sdaftuar)
#15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#15935 (Add /settings.json persistent settings storage by ryanofsky)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)

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.

MarcoFalke force-pushed on Jul 10, 2020

in src/rpc/blockchain.cpp:801 in fa09d3e566 outdated

798@@ -799,7 +799,7 @@ static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
799     if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) {
800         // Block not found on disk. This could be because we have the block
801         // header in our index but don't have the block (for example if a

jnewbery commented at 10:15 am on July 10, 2020:

I think we can just remove everything in the parens here. Presumably if a peer sends us headers and the users requests that block over RPC before we’ve downloaded it we’ll hit this condition too?

MarcoFalke commented at 10:32 am on July 10, 2020:

thanks. Shortened the comment a bit. Also force pushed a typo fix and fixed a test failure.

jnewbery commented at 10:17 am on July 10, 2020: member

ACK fa09d3e5665bf404b7bd388dd881e04d08df13a9.

One comment. Feel free to ignore.

MarcoFalke force-pushed on Jul 10, 2020

jnewbery commented at 12:13 pm on July 10, 2020: member

Code review ACK fa6ec1403bc9cb6b967b1fc380eb8e5d3aed0c0e

doc: Use precise permission flags where possible fab5586122

in test/functional/p2p_permissions.py:111 in fa6ec1403b outdated

107@@ -108,9 +108,9 @@ def check_tx_relay(self):
108         block_op_true = self.nodes[0].getblock(self.nodes[0].generatetoaddress(100, ADDRESS_BCRT1_P2WSH_OP_TRUE)[0])
109         self.sync_all()
110 
111-        self.log.debug("Create a connection from a whitelisted wallet that rebroadcasts raw txs")
112+        self.log.debug("Create a connection from a forecerelay peer that rebroadcasts raw txs")

jonatack commented at 12:33 pm on July 10, 2020:

forcerelay

MarcoFalke commented at 1:49 pm on July 10, 2020:

Thanks, fixed typo

MarcoFalke force-pushed on Jul 10, 2020

jnewbery commented at 1:54 pm on July 10, 2020: member

Did you intentionally revert net_processing.cpp L3575?

MarcoFalke commented at 2:17 pm on July 10, 2020: member

Yes, because it is part of https://github.com/bitcoin/bitcoin/pull/19472/files#diff-eff7adeaec73a769788bb78858815c91R3588 now

No need to make the same changes in two different prs and conflict each other.

jnewbery commented at 2:27 pm on July 10, 2020: member

reACK fab558612278909df93bdf88f5727b04f13aef0f

Yes, because it is part of https://github.com/bitcoin/bitcoin/pull/19472/files#diff-eff7adeaec73a769788bb78858815c91R3588 now

Oh! I just made that change because I assumed that this PR would be merged first and I’d have to rebase anyway.

fjahr commented at 8:08 pm on July 10, 2020: member

Code review ACK fab558612278909df93bdf88f5727b04f13aef0f

MarcoFalke closed this on Jul 10, 2020

MarcoFalke reopened this on Jul 10, 2020

jonatack commented at 7:12 am on July 11, 2020: member

ACK fab558612278909df93bdf88f5727b04f13aef0f

MarcoFalke merged this on Jul 11, 2020

MarcoFalke closed this on Jul 11, 2020

MarcoFalke deleted the branch on Jul 11, 2020

Fabcien referenced this in commit b5e61384aa on Aug 30, 2021

DrahtBot locked this on Feb 15, 2022

doc: Use precise permission flags where possible #19474

Conflicts