Add -blocksonly option #6993

Merged
merged 8 commits into from Nov 14, 2015

Conversation

Projects
None yet
@pstratem
Contributor

pstratem commented Nov 12, 2015

Set fRelay=false and reject all transactions into the mempool

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 12, 2015

Contributor

Concept ACK

Contributor

dcousens commented Nov 12, 2015

Concept ACK

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 12, 2015

Member

Should this force wallet off? But I suppose maybe watch-only wallets don't need to submit txns...

Member

luke-jr commented Nov 12, 2015

Should this force wallet off? But I suppose maybe watch-only wallets don't need to submit txns...

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 12, 2015

Member

@luke-jr Nah! And, in fact with this set you can still submit txns (though it'll be even more obvious its yours!)

Concept Ack. Will test.

Member

gmaxwell commented Nov 12, 2015

@luke-jr Nah! And, in fact with this set you can still submit txns (though it'll be even more obvious its yours!)

Concept Ack. Will test.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 12, 2015

Member

As the code is right now, I don't think you can submit txns...

Member

luke-jr commented Nov 12, 2015

As the code is right now, I don't think you can submit txns...

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 12, 2015

Member

@luke-jr Yup see my comment. :)

Member

gmaxwell commented Nov 12, 2015

@luke-jr Yup see my comment. :)

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 12, 2015

Contributor

@gmaxwell and ruin my record for smallest patch to add a new feature? :)

Contributor

pstratem commented Nov 12, 2015

@gmaxwell and ruin my record for smallest patch to add a new feature? :)

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 12, 2015

Member

Perhaps there is a reason it was record setting? :P

Member

gmaxwell commented Nov 12, 2015

Perhaps there is a reason it was record setting? :P

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 12, 2015

Member

If, instead of blocking at the mempool it did not getdata, then local transactions would work, and you just need to disable the getdata inhibition for whitebinded peers, and the peer is local-- so you'd keep your same lines of code count. Happy?

Member

gmaxwell commented Nov 12, 2015

If, instead of blocking at the mempool it did not getdata, then local transactions would work, and you just need to disable the getdata inhibition for whitebinded peers, and the peer is local-- so you'd keep your same lines of code count. Happy?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 12, 2015

Contributor

The option should be documented, so yet another line is needed :-)

Contributor

paveljanik commented Nov 12, 2015

The option should be documented, so yet another line is needed :-)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 12, 2015

Contributor

What will we do to remote peers relaying transactions to us ignoring we announced that we do not want transactions in version message?

Contributor

paveljanik commented Nov 12, 2015

What will we do to remote peers relaying transactions to us ignoring we announced that we do not want transactions in version message?

@laanwj laanwj added the P2P label Nov 12, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 12, 2015

Member

Concept ACK.

Should this force wallet off? But I suppose maybe watch-only wallets don't need to submit txns...

Otherwise there's already -walletbroadcast=0. To run a wallet without broadcasting.

Member

laanwj commented Nov 12, 2015

Concept ACK.

Should this force wallet off? But I suppose maybe watch-only wallets don't need to submit txns...

Otherwise there's already -walletbroadcast=0. To run a wallet without broadcasting.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 12, 2015

Member

Concept ACK.
Agree with @paveljanik: needs a help message line and maybe a comment on src/net.cpp:L454.

Member

jonasschnelli commented Nov 12, 2015

Concept ACK.
Agree with @paveljanik: needs a help message line and maybe a comment on src/net.cpp:L454.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 12, 2015

Member

Otherwise there's already -walletbroadcast=0

Use parameter interaction and set -walletbroadcast=0 with -blocksonly?

Member

MarcoFalke commented Nov 12, 2015

Otherwise there's already -walletbroadcast=0

Use parameter interaction and set -walletbroadcast=0 with -blocksonly?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 12, 2015

Member

@MarcoFalke Yes.

There was another similar issue raised on IRC: transactions from whitelisted peers. Even though nothing is accepted into the mempool, these are still relayed:

https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4462

Member

laanwj commented Nov 12, 2015

@MarcoFalke Yes.

There was another similar issue raised on IRC: transactions from whitelisted peers. Even though nothing is accepted into the mempool, these are still relayed:

https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4462

@laanwj

View changes

src/main.cpp
@@ -4451,7 +4451,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
assert(recentRejects);
recentRejects->insert(tx.GetHash());
- if (pfrom->fWhitelisted) {
+ if (pfrom->fWhitelisted && GetBoolArg("-whitelistalwaysrelay", true)) {

This comment has been minimized.

@laanwj

laanwj Nov 12, 2015

Member

Please define a static const bool DEFAULT_WHITELISTALWAYSRELAY = true; in main.h and use that
(sorry for making your patch even more hideously huge)

@laanwj

laanwj Nov 12, 2015

Member

Please define a static const bool DEFAULT_WHITELISTALWAYSRELAY = true; in main.h and use that
(sorry for making your patch even more hideously huge)

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 12, 2015

Member

Also, HelpMessage needs update?

@MarcoFalke

MarcoFalke Nov 12, 2015

Member

Also, HelpMessage needs update?

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 12, 2015

Member

@pstratem You can hide them behind showDebug.

@MarcoFalke

MarcoFalke Nov 12, 2015

Member

@pstratem You can hide them behind showDebug.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 12, 2015

Contributor

@MarcoFalke I'm not sure we cant to put either of these options in the HelpMessage

Unless you know what you're doing these are really not a good idea to change.

Contributor

pstratem commented Nov 12, 2015

@MarcoFalke I'm not sure we cant to put either of these options in the HelpMessage

Unless you know what you're doing these are really not a good idea to change.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 12, 2015

Member

@pstratem: for the sake of a central startup arguments "documentation" I think we need to add a help message for every startup parameter. If it's a debugging/expert only feature, it should be placed behind the -debughelp wall.

Member

jonasschnelli commented Nov 12, 2015

@pstratem: for the sake of a central startup arguments "documentation" I think we need to add a help message for every startup parameter. If it's a debugging/expert only feature, it should be placed behind the -debughelp wall.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 12, 2015

Contributor

We really should define something like NODE_BLOCKS and NODE_TXS... Doesn't necessarily need to be in this pull, but worth noting.

Contributor

petertodd commented Nov 12, 2015

We really should define something like NODE_BLOCKS and NODE_TXS... Doesn't necessarily need to be in this pull, but worth noting.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 13, 2015

Contributor

@petertodd agreed

Contributor

pstratem commented Nov 13, 2015

@petertodd agreed

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 13, 2015

Member

Needs rebase. And my tested/code ack is held only on the fact that there is a global static setting for the default thats not used everywhere. :(

Member

gmaxwell commented Nov 13, 2015

Needs rebase. And my tested/code ack is held only on the fact that there is a global static setting for the default thats not used everywhere. :(

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 14, 2015

Member

There was also talk about sending an empty filterload to peers in this mode, if they support NODE_BLOOM.
Let's do this (if still relevant) in a later pull, just mentioning it for completeness.

Edit: utACK

Member

laanwj commented Nov 14, 2015

There was also talk about sending an empty filterload to peers in this mode, if they support NODE_BLOOM.
Let's do this (if still relevant) in a later pull, just mentioning it for completeness.

Edit: utACK

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 14, 2015

Member

@laanwj We don't need to send an empty filterload because the relay flag has the same effect! (and doesn't require NODE_BLOOM support)

Member

gmaxwell commented Nov 14, 2015

@laanwj We don't need to send an empty filterload because the relay flag has the same effect! (and doesn't require NODE_BLOOM support)

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 14, 2015

Member

@pstratem Move the DEFAULT_BLOCKSONLY to net.h, then you can use it everywhere uniformly and future programmers will not hate you quite so much. (thanks to @laanwj for asking the dumb question I missed. :) )

Member

gmaxwell commented Nov 14, 2015

@pstratem Move the DEFAULT_BLOCKSONLY to net.h, then you can use it everywhere uniformly and future programmers will not hate you quite so much. (thanks to @laanwj for asking the dumb question I missed. :) )

@gmaxwell gmaxwell merged commit bbf49da into bitcoin:master Nov 14, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

gmaxwell added a commit that referenced this pull request Nov 14, 2015

Merge pull request #6993
bbf49da Fix comment for blocksonly parameter interactions (Patick Strateman)
6a4982f Fix fRelayTxs comment (Patick Strateman)
59441a0 Display DEFAULT_WHITELISTALWAYSRELAY in help text (Patick Strateman)
71a2683 Use DEFAULT_BLOCKSONLY and DEFAULT_WHITELISTALWAYSRELAY constants (Patick Strateman)
762b13b Add help text for blocksonly and whitelistalwaysrelay (Patick Strateman)
3a96497 Add whitelistalwaysrelay option (Patick Strateman)
420fa81 Do not process tx inv's in blocksonly mode (Patick Strateman)
4044f07 Add blocksonly mode (Patick Strateman)
@dajohi

This comment has been minimized.

Show comment
Hide comment
@dajohi

dajohi Nov 17, 2015

Contributor

should you also ignore tx messages?

Contributor

dajohi commented on 420fa81 Nov 17, 2015

should you also ignore tx messages?

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 17, 2015

Contributor

@dajohi yes, i'll create a new pr

Contributor

pstratem replied Nov 17, 2015

@dajohi yes, i'll create a new pr

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Mar 16, 2016

Contributor

Erk.. This is really ugly... And the pull doesn't even state who needs this functionality or why.

Contributor

rebroad commented Mar 16, 2016

Erk.. This is really ugly... And the pull doesn't even state who needs this functionality or why.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 16, 2016

Member

@rebroad Basically this makes Core a blockchain-only node and disables off-chain transactions.

Member

luke-jr commented Mar 16, 2016

@rebroad Basically this makes Core a blockchain-only node and disables off-chain transactions.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 16, 2016

Member

@rebroad It's fairly self explanatory. If you want to run a node that only processes blocks (e.g. don't care about unconfirmed transactions); you can set this and save ~80% of your bandwidth, including all redundant transmission of block data; "Blocksonly". This is good for many applications (including my laptop... :) )

Member

gmaxwell commented Mar 16, 2016

@rebroad It's fairly self explanatory. If you want to run a node that only processes blocks (e.g. don't care about unconfirmed transactions); you can set this and save ~80% of your bandwidth, including all redundant transmission of block data; "Blocksonly". This is good for many applications (including my laptop... :) )

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Mar 17, 2016

Contributor

Ah, I thought I had raised a pull request for my Antisocial fork, but it seems not. I think it would be useful to have an option to do blocksonly during IBD only. Would this be of interest to anyone?

Contributor

rebroad commented Mar 17, 2016

Ah, I thought I had raised a pull request for my Antisocial fork, but it seems not. I think it would be useful to have an option to do blocksonly during IBD only. Would this be of interest to anyone?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 17, 2016

Member

to do blocksonly during IBD only

@rebroad That should already be the case? Most node functionality, such as requesting transactions, is disabled during IBD

Member

laanwj commented Mar 17, 2016

to do blocksonly during IBD only

@rebroad That should already be the case? Most node functionality, such as requesting transactions, is disabled during IBD

@str4d str4d referenced this pull request in zcash/zcash Jul 14, 2017

Open

Bitcoin 0.12 P2P/Net PRs 1 #2534

zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018

Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment