Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Update init and reduce-traffic docs for -blocksonly #18391

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

glowang
Copy link
Contributor

@glowang glowang commented Mar 20, 2020

When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0.

This behavior is not captured by the current documentation, which
claims that -blocksonly does not impact any wallet transactions at
all.

Fixes #17294

@fanquake fanquake added the Docs label Mar 20, 2020
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The walletbroadcast is only set "softly" to 0, i.e. when it was not set otherwise (in the config or on the command line)
  • RPC transactions are relayed as far as I can tell, see also the second section in the test ./test/functional/p2p_blocksonly.py
  • It would be nice to extend that test for wallet transactions and maybe even whitelisted peers. (Not necessarily in this pull, it can also be done later)

@maflcko maflcko added this to the 0.20.0 milestone Mar 20, 2020
@andrewtoth
Copy link
Contributor

Lines 6 and 29 of reduce-traffic.md still show the outdated number of 8 outgoing peers, but this has been changed to 10. Since you're modifying that file you could update those numbers as well.

@glowang
Copy link
Contributor Author

glowang commented Mar 25, 2020

  • The walletbroadcast is only set "softly" to 0, i.e. when it was not set otherwise (in the config or on the command line)
  • RPC transactions are relayed as far as I can tell, see also the second section in the test ./test/functional/p2p_blocksonly.py
  • It would be nice to extend that test for wallet transactions and maybe even whitelisted peers. (Not necessarily in this pull, it can also be done later)

Are you referring to “wallet transaction" as a loose term? I thought wallet transaction includes the ones initiated via rpc already? @MarcoFalke

I would love to add more tests as you mentioned in a follow up PR!

doc/reduce-traffic.md Outdated Show resolved Hide resolved
doc/reduce-traffic.md Outdated Show resolved Hide resolved
doc/reduce-traffic.md Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Mar 25, 2020

Thanks for improving the documentation!
ACK minus nits

@laanwj
Copy link
Member

laanwj commented Mar 27, 2020

ACK 869327c

doc/reduce-memory.md Outdated Show resolved Hide resolved
@glowang glowang force-pushed the update_blocksonly_docs branch 4 times, most recently from 92255ae to b4a976d Compare March 29, 2020 07:32
@glowang glowang requested review from maflcko and laanwj March 29, 2020 07:33
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK after the code change is removed

src/net.h Outdated
@@ -64,7 +64,7 @@ static const unsigned int MAX_SUBVERSION_LENGTH = 256;
/** Maximum number of automatic outgoing nodes over which we'll relay everything (blocks, tx, addrs, etc) */
static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
/** Maximum number of addnode outgoing nodes */
static const int MAX_ADDNODE_CONNECTIONS = 8;
static const int MAX_ADDNODE_CONNECTIONS = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes (behaviour changes) should probably be in a separate pull request from documentation changes.

When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0 if it has not been set already.This behavior
is not captured by the current documentation, which claims that -blocksonly
does not impact any wallet transactions.

Update the max number of outgoing peers from 8 to 10, due to the
addition of two -blocksonly peers.
@glowang glowang requested a review from maflcko March 29, 2020 12:14
@maflcko
Copy link
Member

maflcko commented Mar 29, 2020

ACK 621e86e

@maflcko maflcko merged commit 6cfb3db into bitcoin:master Mar 29, 2020
@@ -24,8 +24,7 @@ The size of some in-memory caches can be reduced. As caches trade off memory usa

## Number of peers

- `-maxconnections=<n>` - the maximum number of connections, this defaults to `125`. Each active connection takes up some memory. Only significant if incoming
connections are enabled, otherwise the number of connections will never be more than `8`.
- `-maxconnections=<n>` - the maximum number of connections, this defaults to `125`. Each active connection takes up some memory. Only significant if incoming connections are enabled, otherwise the number of connections will never be more than `10`. Of the 10 outbound peers, there can be 8 full outgoing connections and 2 -blocksonly peers, in which case they are block/addr peers, but not tx peers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoopsie, just realized that blocksonly is different from block-relay-only. Fixed in #18464

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 14, 2021
Summary:
> When -blocksonly is set to 1, it interacts with the -walletbroadcast
> parameter and sets it to 0 if it has not been set already.This behavior
> is not captured by the current documentation, which claims that -blocksonly
> does not impact any wallet transactions.
>
> Update the max number of outgoing peers from 8 to 10, due to the
> addition of two -blocksonly peers.

This is a backport of Core [[bitcoin/bitcoin#18391 | PR18391]] and [[bitcoin/bitcoin#18464 | PR18464]]

Test Plan: `ninja && src/bitcoind -help`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8894
zkbot added a commit to zcash/zcash that referenced this pull request Mar 31, 2021
Add -blocksonly option

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6993
- bitcoin/bitcoin#7046
- bitcoin/bitcoin#6780
  - The third commit (we backported the rest in #2390).
- bitcoin/bitcoin#7126
- bitcoin/bitcoin#7439
- bitcoin/bitcoin#15990
  - Only the `-blocksonly` documentation changes.
- bitcoin/bitcoin#16555
- bitcoin/bitcoin#18391
  - Only the `-blocksonly` documentation changes.

Part of #2074.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

code for blocksonly disagrees with documentation
5 participants