Allow block responses to getblocktxn requests #469
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Should probably finalise this BIP soon, though, and future change proposals go into a new BIP...? |
|
Since my The problem is the "MUST" in "the sender of such a message a cmpctblock for the block hash identified in this message MUST respond" clause which we don't currently follow now because of the @TheBlueMatt, do you have any suggestions on how to fix this? The minimal fix would simply be to replace the MUST above with SHOULD, but maybe you have a good way of characterizing the cases where you don't think a getblocktxn request should get a response, and would want to mention them in the BIP. Another option would be to make no changes to the BIP at all and simply make bitcoind do what the current wording says it should do. This is what my alternate fix does (ryanofsky/bitcoin@fix-8842-getblocktxnmin). |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Nov 7, 2016
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced.
sdaftuar
reviewed
Nov 11, 2016
| @@ -128,7 +128,7 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde | |||
|
|
|||
| ====getblocktxn==== | |||
| # The getblocktxn message is defined as as a message containing a serialized BlockTransactionsRequest message and pchCommand == "getblocktxn". | |||
| # Upon receipt of a properly-formatted getblocktxnmessage, nodes which recently provided the sender of such a message a cmpctblock for the block hash identified in this message MUST respond with an appropriate blocktxn message. Such a blocktxn message MUST contain exactly and only each transaction which is present in the appropriate block at the index specified in the getblocktxn indexes list, in the order requested. | |||
| # Upon receipt of a properly-formatted getblocktxnmessage, nodes which provided the sender of such a message a cmpctblock for the block hash identified in this message MUST respond with either an appropriate blocktxn message, or a full block message. A blocktxn response MUST contain exactly and only each transaction which is present in the appropriate block at the index specified in the getblocktxn indexes list, in the order requested. | |||
There was a problem hiding this comment.
I think if you add "recently" back in (so "nodes which recently provided the sender of such a message..."), then we could call it a day...
There was a problem hiding this comment.
Good call. Done. I have no idea why I removed the word "recently" to begin with...
ryanofsky
force-pushed
the
getblocktxn-nonrecent
branch
from
b9e74e6
to
443ef8c
Compare
|
I was reading the implementation and I think MUST is still fine... There is still a corner case if the block isnt on the main chain and is a month old, but I think thats close enough. |
|
Yes, MUST is fine because the word "recently" is there to qualify it and make it mushier. When I made the comments above, I forgot "recently" was in the original text and didn't realize I had removed it. Anyway, sounds like this is ready to be merged now. |
TheBlueMatt
reviewed
Nov 12, 2016
| @@ -128,7 +128,7 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde | |||
|
|
|||
| ====getblocktxn==== | |||
| # The getblocktxn message is defined as as a message containing a serialized BlockTransactionsRequest message and pchCommand == "getblocktxn". | |||
| # Upon receipt of a properly-formatted getblocktxnmessage, nodes which recently provided the sender of such a message a cmpctblock for the block hash identified in this message MUST respond with an appropriate blocktxn message. Such a blocktxn message MUST contain exactly and only each transaction which is present in the appropriate block at the index specified in the getblocktxn indexes list, in the order requested. | |||
| # Upon receipt of a properly-formatted getblocktxnmessage, nodes which recently provided the sender of such a message a cmpctblock for the block hash identified in this message MUST respond with either an appropriate blocktxn message, or a full block message. A blocktxn response MUST contain exactly and only each transaction which is present in the appropriate block at the index specified in the getblocktxn indexes list, in the order requested. | |||
There was a problem hiding this comment.
Oops, missing space between getblocktxn and message (not new, but should still be fixed).
There was a problem hiding this comment.
Fixed the missing space.
Corresponding bitcoind change in: dac53b5 Modify getblocktxn handler not to drop requests for old blocks
ryanofsky
force-pushed
the
getblocktxn-nonrecent
branch
from
443ef8c
to
376029b
Compare
|
ACK On November 14, 2016 8:00:29 AM PST, Russell Yanofsky notifications@github.com wrote:
|
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Nov 20, 2016
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Github-Pull: bitcoin#9058 Rebased-From: dac53b5 Github-Pull: bitcoin#9160 Rebased-From: ec34648
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 3, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 3, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 3, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 3, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 6, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jan 11, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jun 7, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jun 7, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jun 8, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dagurval
pushed a commit
to dagurval/bitcoinxt
that referenced
this pull request
Jun 8, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
dgenr8
pushed a commit
to dgenr8/bitcoinxt
that referenced
this pull request
Jun 29, 2017
The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in bitcoin/bitcoin#8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at bitcoin/bips#469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Conflicts: src/main.cpp
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Corresponding bitcoind change is: bitcoin/bitcoin#9058 commit
Modify getblocktxn handler not to drop requests for old blocks.