Allow block responses to getblocktxn requests #469

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:getblocktxn-nonrecent changing 1 files +1 −1
  1. ryanofsky commented at 2:53 PM on November 1, 2016: contributor

    Corresponding bitcoind change is: https://github.com/bitcoin/bitcoin/pull/9058 commit Modify getblocktxn handler not to drop requests for old blocks.

  2. ryanofsky cross-referenced this on Nov 1, 2016 from issue Fixes for p2p-compactblocks.py test timeouts on travis (#8842) by ryanofsky
  3. luke-jr added the label Proposed BIP modification on Nov 2, 2016
  4. luke-jr commented at 4:32 AM on November 2, 2016: member

    @TheBlueMatt

    Should probably finalise this BIP soon, though, and future change proposals go into a new BIP...?

  5. ryanofsky referenced this in commit f6c7943782 on Nov 3, 2016
  6. ryanofsky commented at 9:03 PM on November 3, 2016: contributor

    Since my Modify getblocktxn handler not to drop requests for old blocks commit in https://github.com/bitcoin/bitcoin/pull/9058 was changed to use ProcessGetData instead of doing a direct PushMessage call, @sdaftuar pointed out that the currently proposed wording in this BIP no longer matches the behavior implemented there.

    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 MAX_BLOCKTXN_DEPTH check, and will continue to not follow in the future because of ProcessGetData heuristics. @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 (https://github.com/ryanofsky/bitcoin/commit/fix-8842-getblocktxnmin).

  7. ryanofsky referenced this in commit dac53b58b5 on Nov 7, 2016
  8. in bip-0152.mediawiki:None in b9e74e630a outdated 
     127 | @@ -128,7 +128,7 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde
 128 |  
 129 |  ====getblocktxn====
 130 |  # The getblocktxn message is defined as as a message containing a serialized BlockTransactionsRequest message and pchCommand == "getblocktxn".
 131 | -# 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.
 132 | +# 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.
    sdaftuar commented at 4:39 PM on November 11, 2016:

    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...

    ryanofsky commented at 5:11 PM on November 11, 2016:

    Good call. Done. I have no idea why I removed the word "recently" to begin with...

  9. ryanofsky force-pushed on Nov 11, 2016
  10. TheBlueMatt commented at 12:29 AM on November 12, 2016: contributor

    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.

  11. ryanofsky commented at 12:06 PM on November 12, 2016: contributor

    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.

  12. in bip-0152.mediawiki:None in 443ef8c84d outdated 
     127 | @@ -128,7 +128,7 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde
 128 |  
 129 |  ====getblocktxn====
 130 |  # The getblocktxn message is defined as as a message containing a serialized BlockTransactionsRequest message and pchCommand == "getblocktxn".
 131 | -# 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.
 132 | +# 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.
    TheBlueMatt commented at 10:02 PM on November 12, 2016:

    Oops, missing space between getblocktxn and message (not new, but should still be fixed).

    ryanofsky commented at 4:00 PM on November 14, 2016:

    Fixed the missing space.

  13. TheBlueMatt commented at 10:02 PM on November 12, 2016: contributor

    Would be nice to fix the grammar bug, but otherwise ACK.

  14. BIP152: Fix missing space between words. 21985daefb
  15. BIP152: Allow block responses to getblocktxn requests
    Corresponding bitcoind change in:

dac53b5 Modify getblocktxn handler not to drop requests for old blocks
    376029beea
  16. ryanofsky force-pushed on Nov 14, 2016
  17. TheBlueMatt commented at 6:48 PM on November 14, 2016: contributor

    ACK

    On November 14, 2016 8:00:29 AM PST, Russell Yanofsky notifications@github.com wrote:

    ryanofsky commented on this pull request.

    @@ -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.

    Fixed the missing space.

    You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #469

  18. luke-jr merged this on Nov 15, 2016
  19. luke-jr closed this on Nov 15, 2016
  20. MarcoFalke referenced this in commit 573b575174 on Nov 20, 2016
  21. MarcoFalke referenced this in commit c65fb7c3ff on Nov 20, 2016
  22. MarcoFalke referenced this in commit e8461666ec on Nov 20, 2016
  23. dagurval referenced this in commit 81ba25c7cd on Jan 3, 2017
  24. dagurval referenced this in commit 4f1f2b713c on Jan 3, 2017
  25. dagurval referenced this in commit 10b20ffd6b on Jan 3, 2017
  26. dagurval referenced this in commit 3a4ca4106b on Jan 3, 2017
  27. dagurval referenced this in commit bf6e9543ce on Jan 6, 2017
  28. dagurval referenced this in commit 75de4074f0 on Jan 11, 2017
  29. dagurval referenced this in commit 1d6e228483 on Jun 7, 2017
  30. dagurval referenced this in commit 45a78b33d0 on Jun 7, 2017
  31. dagurval referenced this in commit d53079c7e0 on Jun 8, 2017
  32. dagurval referenced this in commit 3b0f998877 on Jun 8, 2017
  33. dgenr8 referenced this in commit 44931ccce6 on Jun 29, 2017
Contributors
ryanofskyluke-jrsdaftuarTheBlueMatt
Labels
Proposed BIP modification
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 11:10 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me