MSG_CMPCT_BLOCK enum value is already in use #8500

issue dagurval opened this issue on August 11, 2016
  1. dagurval commented at 8:52 PM on August 11, 2016: contributor

    Compact blocks MSG_CMPCT_BLOCK enum value is already in use. It is used as MSG_THINBLOCK for xthin blocks.

    The existing type MSG_THINBLOCK is already in use and widely deployed: In Bitcoin XT, Bitcoin Classic and Bitcoin Unlimited.

    Please consider changing the enum value to an available one. I plan to add compact blocks to XT and I'd rather not have to guess the enum values meaning from context.

  2. achow101 commented at 9:12 PM on August 11, 2016: member

    When did xthin first define that enum? Was it before or after Compact Blocks defined it? Obviously the one that defined it first should get that enum and the one that did later should take another one.

  3. luke-jr commented at 9:16 PM on August 11, 2016: member

    Sigh, avoiding stuff like this is why the implementation-independent BIP process exists. And yet even Bitcoin Unlimited's BUIP 010 doesn't make any mention of using an inv enum value.

    Obviously alone it won't be enough to fix the issue(s) herein, but can you perhaps ask someone to draft a BIP describing at a minimum this usage? Mr. Stone started one back in March, but it made no mention of the inv enum.

  4. jameshilliard commented at 9:18 PM on August 11, 2016: contributor

    Looks like this is conflicting with MSG_THINBLOCK and not MSG_XTHINBLOCK, it seems only MSG_XTHINBLOCK is the one in use for the most part though, if that's the case you should just be able to switch out MSG_THINBLOCK for MSG_CMPCT_BLOCK.

  5. gandrewstone commented at 12:19 AM on August 12, 2016: none

    My BIP was not even assigned a number so I did not bother to update it with the final implementation details. I'd be happy to do so now if it will receive a number

  6. jameshilliard commented at 12:30 AM on August 12, 2016: contributor

    @gandrewstone From what I recall you refused to submit a BIP according to the BIP process.

  7. achow101 commented at 12:32 AM on August 12, 2016: member

    That's not how the BIP process works. You have to finish it first in order to show people to study it and offer criticisms and suggestions before it is assigned a number.

    Furthermore, you explicitly rejected the offer to make it a BIP and stated that you did not want to go through the process of making it a BIP.

    On August 11, 2016 8:19:51 PM gandrewstone notifications@github.com wrote:

    My BIP was not even assigned a number so I did not bother to update it with the final implementation details. I'd be happy to do so now if it will receive a number

    You are receiving this because you commented. Reply to this email directly or view it on GitHub: #8500 (comment)

  8. luke-jr commented at 12:33 AM on August 12, 2016: member

    @gandrewstone I see no reason it should not be assigned a number at this point. Let's go with BIP 153. @jameshilliard That was my proposal to issue BIP numbers to all the BUIPs. He did submit it by the BIP process, just I got sidetracked after the dev ML discussion started. My fault entirely for not assigning the number for his proposal.

    In any case, no need to further discuss BIP 153 specific stuff here; let's leave this for the conflict issue, and take non-conflict stuff back to the dev ML.

  9. dagurval commented at 12:48 PM on August 13, 2016: contributor

    @achow101: xthin was included in a BU release in March . A second implementation (not a merge) was released by XT a month ago. FWIW, first prototype was in Dec, 2015.

    I agree the BUIP should mention this enum. Lessons learned I suppose. It's also IMHO a good idea to at least peek at the existing wheel when re-inventing a new one. Extending this enum is an obvious way to request a new block type and xthin is not exactly a hidden gem. @jameshilliard: Nodes supporting xthin need to respond correctly to MSG_THINBLOCK - even though MSG_XTHINBLOCK is mostly used.

    If Core cannot change to an available enum value it’s probably possible to derive its meaning from context. If a node has sent “sendcmpct” it means MSG_CMPCT_BLOCK, if not then it means MSG_THINBLOCK. Both the BIP and the BUIP could be updated to reflect this.

  10. laanwj commented at 1:22 PM on August 13, 2016: member

    Deliberate or not, this is a process failure. It is too late to change the enum now, this should have been indicated during the review phase for BIP152 which mentions this:

    A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are added: sendcmpct, cmpctblock, getblocktxn, and blocktxn.

    Also the number for MSG_THINBLOCK should have been mentioned in a BIP so that reviewers could have been aware of it. The purpose of BIPs is to provide network-wide coordination exactly for cases such as this. If people don't follow the process there are bound to be collisions such as this.

    My BIP was not even assigned a number so I did not bother to update it with the final implementation details. I'd be happy to do so now if it will receive a number

    Numbers are only assigned after a draft BIP is written. See BIP1.

    Closing this issue as there is nothing to be done here. Please let it be a learning experience for next time. We don't have a lot of process in Bitcoin development, but the little bit that exists tends to exists for a good reason.

  11. laanwj closed this on Aug 13, 2016

  12. zander commented at 7:53 PM on August 13, 2016: none

    This is indeed a process failure, that is correct.

    Good thing that this is filed in your bug report system while you are in your release-candidate process.

    This is a showstopper because it will cause problems for bitcoin nodes communicating together. it breaks the p2p layer!. It is irrelevant to blame anyone, arguments really go both ways. As I said, it is good that this was caught in the RC time.

    I think if you ignore this valuable feedback on your competing design, while there is a clear demonstration that your not-yet-released software can't interoperate with something that has been on the market for 6 months, you are creating a disruption to the Bitcoin ecosystem and on top of that you should be ashamed of yourselves.

  13. gmaxwell commented at 8:43 PM on August 13, 2016: contributor

    I don't see anything wrong here. I think the issue was incorrectly opened.

    Yes, they use the same enum. So what? Support for BIP152 is negotiated. As far as I know the only consequence of the index reuse is that an implementation couldn't support both protocols simultaneously on a single connection. That is it. It would make no logical sense to run both since they both accomplish the same thing (though BIP152 does so more simply and efficiently).

    If "xthin" actually had a written specification or had this concern otherwise been raised earlier I would have argued that both should use the same enum, as they both do the same thing and they both couldn't logically be used at the same time. So through xthin's lack of documentation we ended up with a result that was the same as what I would have argued for had the question been raised.

    The issue is doubly moot when, presumably due to a lack of a specification, the existing "xthin" implementations are already incompatible with each other. (e.g. https://github.com/bitcoinclassic/bitcoinclassic/commit/833cd35dcaa79fa3e91f271458ce44c085ea60a2 ); and if something did have to change-- right now I observe five currently-BIP152-using peers for every XT+Classic+Unlimited peer (and only 'unlimited' even has released support for 'xthin').

  14. zander commented at 9:13 PM on August 13, 2016: none

    and if something did have to change-- right now I observe five currently-BIP152-using peers for every XT+Classic+Unlimited peer (and only 'unlimited' even has released support for 'xthin').

    I repeat myself, you should be ashamed of yourselves.

    Having a bigger market share doesn't give you the right to tell others to recall their products already 6 months on the market because you are too lazy to do fix yours before your release.

  15. achow101 commented at 9:29 PM on August 13, 2016: member

    @dagurval How does xthin negotiate that xthin should be used for transmitting blocks? As @gmaxwell said above, it would be pointless to use both xthin and compact blocks on the same connection, so all you need to know is which protocol to use for each connection. I don't see why the same enum can't be used if the node explicitly indicates which protocol it wants to use for transmitting blocks.

  16. kanzure commented at 9:59 PM on August 13, 2016: contributor

    So, someone was asked to write a specification, and a draft was emailed around, and it excluded a specification detail which you are now arguing should have been known because somehow readers were supposed to know the value that was not included? ok

    I see at least two opportunities here that are overlooked. @gandrewstone emailed a draft around, which did not include the enum value, a regrettable situation, but at least he gave a good faith effort in that direction. Secondly, there was also public peer review and code review for the bip152 proposal, bip152 draft, and a public implementation of bip152 for code review as well, all of which were excellent opportunities for collaboration and analysis of any perceived potential problems.

    Thankfully, it seems like this is not an actual issue, as discussed above, such as in terms of the network layer. However, this serves as an interesting example of how it's useful to communicate with other Bitcoin developers through BIPs and other communication.

    I am not presently convinced regarding the claims of "asked to recall my products from a market" and "p2p layer breakage" and "showstopper". It's the same user, and I note the absence of any such request from anyone else in the thread. Readers of this issue might be interested to note that a social media outlet has targeted this issue page, and that the thread could be experiencing the effects of brigading as a result.

  17. krisives commented at 10:28 PM on August 13, 2016: none

    Has the Bitcoin development community broken down to the point where we can't even agree on enum values to use? This is a serious question and I think this issue illustrates it. If I read this ticket correctly the current solution is "it can't be solved?" as a whole?

  18. gandrewstone commented at 10:31 PM on August 13, 2016: none

    I think that is an unfortunate but not so big deal based on the particulars of this situation (due to the services bit), but let's try to do better the next time around. In that spirit, we will submit a BIP for common resources that we require since Core is the upstream implementation.

    However, on your part I hope that you recognise that our process is different than yours and we feel no need to justify, explain, or ask permission for our use of these resources within your BIP process. So I hope that you guys will trust that we will not make frivolous use of these common resources and instead accept "we need XXX resource" BIPs not as a technical proposal to be debated (and in the prior case, rejected instantly) but more as an informative statement of intention.

  19. laanwj commented at 1:39 AM on August 14, 2016: member

    I think you misunderstand. It's not our BIP process. The BIP process is for all development on the Bitcoin network, including yours. You may feel so different and completely separate from anything else, but as long as you want to communicate on the same network it makes sense to coordinate.

    If the BIP process fails to coordinate the entire Bitcoin development community , it is as good as useless and we can just as well abandon it and we would be better of using a random number generator to assign numbers and hope for the best, in the future.

  20. laanwj locked this on Aug 14, 2016

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me