Skip to content

Conversation

@mikehearn
Copy link
Contributor

It is sent if any data that isn't in the relayable set is requested.

…at aren't in the relayable set are requested.
@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/903d146030e741441c288873ef3c682fb5019101 for binaries and test log.

@jgarzik
Copy link
Contributor

jgarzik commented Jan 19, 2013

This replaces a ping-after-getdata, correct?

@mikehearn
Copy link
Contributor Author

Yeah. I have code that can do both. It's a part of resolving the issues Peter Todd raised, from the bitcoinj side.

@gavinandresen
Copy link
Contributor

Could an attacker fill up memory using vNotFound?

@sipa
Copy link
Member

sipa commented Jan 21, 2013

There's no way for it to grow larger than the original getdata request, is there?

@gavinandresen
Copy link
Contributor

ACK, no vulnerability because the response will always be smaller than the request.

@gavinandresen
Copy link
Contributor

Quibble on the naming: "notfound" is pretty generic. Maybe "inv_unknown" ?

@mikehearn
Copy link
Contributor Author

There's a limit on how long message IDs can be :( inv_unknown is 11 so it just fits, but that naming scheme would be constraining in future.

Really, the whole protocol needs to get more explicit. This is just one example of a general problem.

How about we introduce a convention for cases where a command results in an error. e:getdata?

@jgarzik
Copy link
Contributor

jgarzik commented Jan 21, 2013

How about we introduce a convention where all situations that might result in a response do receive a response.

@mikehearn
Copy link
Contributor Author

I'm all for that, but it has to be done piece-wise. This is one chunk of it.

@sipa
Copy link
Member

sipa commented Jan 22, 2013

ACK on this change, but I disagree with requiring a response for everything.

In its core, the P2P protocol is a gossip system. Nodes make sure they tell eachother about stuff they learn, and request what they don't know. This network layer caches and bundles requests, prevents duplicates, re-requests if necessary, .... Turning that into a request-response system would be wasting bandwith and performance. A fully fledged message-response system also needs things like request ID's that get repeated in responses, as otherwise an announcement can be confused to be a response to something else (for example, invs can be both sent as response to a getblocks, or as an announcement for a new best block - addr can be a response to getaddr or an announcement). I think there's just more to it than "making everything that might result a response do receive a response".

On the other hand, for some things, this would make perfect sense. getdata obviously needs a response, and so does version (verack). It just needs to be dealt with on a case-by-case basis.

@gavinandresen
Copy link
Contributor

Merging now. Can tweak later.

gavinandresen added a commit that referenced this pull request Jan 23, 2013
Add a notfound message to getdata.
@gavinandresen gavinandresen merged commit a337505 into bitcoin:master Jan 23, 2013
@mikehearn mikehearn deleted the notfoundmsg branch February 24, 2013 18:34
@jgarzik
Copy link
Contributor

jgarzik commented Mar 30, 2013

I still wonder if this wants a BIP.

@rebroad
Copy link
Contributor

rebroad commented Apr 2, 2013

This should certainly require a BIP. As a side note, I wonder if adding a timeout value to getdata is worthwhile also - i.e. most nodes wait 2 minutes before requesting a tx or block elsewhere. If a node responds more than 2 hours later it's only likely to be sending something the node already has. a timeout would allow the node to decide whether the requesting node is still interested.

laudney pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request Mar 19, 2014
Add a notfound message to getdata.
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 31, 2018
@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.

7 participants