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

Stop processing messages on full send buffer and dont disconnect. #973

Merged
merged 1 commit into from
Jun 27, 2012

Conversation

TheBlueMatt
Copy link
Contributor

Also decrease default send/receive buffer sizes from 10 to 5 mb
as this patch makes it easy for a node to fill both instead of
only send.

The largest advantage is not having to read from disk or serialize blocks on getblocks requests.

@TheBlueMatt
Copy link
Contributor Author

I did test downloading from block 143XXX to current tip and didnt see any issues (nor even any warnings about send buffer being full, which makes sense given that this is downloading from one local node to another)

@jgarzik
Copy link
Contributor

jgarzik commented Mar 22, 2012

Looks like fun stuff to play around with... building now.

Disagree with the minor AddAddress debug change. Once a node is running, these messages are (a) minimal and (b) useful. Better approach is to start separating messages at a much more fine grain, by introducing, e.g. fNetDebug (network-related messages) and fTxDebug (tx/mempool-related messages)

@TheBlueMatt
Copy link
Contributor Author

Everything short of the last commit is part of #771 and is just here because I had that branch checked out when I wrote this, plus I dont expect this to get merged until 0.7 anyway, so there is no point doing it on master.
And agreed on the more finely controlled Debug messages, but AddAddress gets really annoying when you are reading debug.log right after startup (which, if you have an error, its likely to occur closer to startup anyway) Maybe one printf per AddAddress...

@sipa
Copy link
Member

sipa commented Mar 22, 2012

@TheBlueMatt addrman has significantly less address messages (it adds groups of incoming addrs at once).

@TheBlueMatt
Copy link
Contributor Author

Ah, well Ill go drop that commit from CBlockStore then.

@Diapolo
Copy link

Diapolo commented Mar 22, 2012

The strings explaining -maxreceivebuffer and -maxsendbuffer should be updated to reflect the change of the default size.

@TheBlueMatt
Copy link
Contributor Author

Oops, good point...fixed.

@Diapolo
Copy link

Diapolo commented Mar 22, 2012

Helping and get helped, right ;).

@TheBlueMatt TheBlueMatt reopened this Mar 27, 2012
@gavinandresen
Copy link
Contributor

The "don't disconnect" part of this change worries me.

If an attacker can connect to you 100 times from 100 different IP addresses and then fill up the send buffer on each connection that's bad.

I suppose they could try to fill up your send buffer minus 1 byte now...

@TheBlueMatt
Copy link
Contributor Author

This patch doesnt change the difficulty of eating 10MB of any node's ram. Also, I dont see much option in the way of disconnecting nodes for eating too much ram in buffers, we may end up just killing nodes behind 24.4k modems. Or atleast I dont trust myself enough to write any kind of such code.

@jgarzik
Copy link
Contributor

jgarzik commented May 8, 2012

ACK

@TheBlueMatt
Copy link
Contributor Author

Rebased.

@sipa
Copy link
Member

sipa commented Jun 12, 2012

What if a node sends you a good new latest best block while the send buffer to that node is full, and then disconnects?

Probably not a real issue, and I certainly likes this approach more than the earlier hack to cut off the 500 block invs earlier when size exceeds the buffer.

@sipa
Copy link
Member

sipa commented Jun 14, 2012

ACK

Also decrease default send/receive buffer sizes from 10 to 5 mb
as this patch makes it easy for a node to fill both instead of
only send.
@gavinandresen
Copy link
Contributor

ACK

sipa added a commit that referenced this pull request Jun 27, 2012
Stop processing messages on full send buffer and dont disconnect.
@sipa sipa merged commit 6c88568 into bitcoin:master Jun 27, 2012
coblee referenced this pull request in litecoin-project/litecoin Jul 17, 2012
Stop processing messages on full send buffer and dont disconnect.
jwrb pushed a commit to jwrb/paycoin that referenced this pull request Sep 17, 2015
Decrease default send/revieve buffer from 10MB to 1MB as this patch makes it easy for a node to fill both instead of only send.
Ported from [pull 973](bitcoin/bitcoin#973) and [pull 1545](bitcoin/bitcoin#1545)

The largest advantage is not having to read from disk or serialize blocks on getblocks requests.
jwrb pushed a commit to jwrb/paycoin that referenced this pull request Oct 22, 2015
Decrease default send/revieve buffer from 10MB to 1MB as this patch makes it easy for a node to fill both instead of only send.
Ported from [pull 973](bitcoin/bitcoin#973) and [pull 1545](bitcoin/bitcoin#1545)

The largest advantage is not having to read from disk or serialize blocks on getblocks requests.
jwrb pushed a commit to jwrb/paycoin that referenced this pull request Oct 22, 2015
Decrease default send/revieve buffer from 10MB to 1MB as this patch makes it easy for a node to fill both instead of only send.
Ported from [pull 973](bitcoin/bitcoin#973) and [pull 1545](bitcoin/bitcoin#1545)

The largest advantage is not having to read from disk or serialize blocks on getblocks requests.
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants