BIP330: drop redundant booleans from the sendtxrcncl message #1376

pull vasild wants to merge 1 commits into bitcoin:master from vasild:bip330_sendtxrcncl_smplfctn changing 1 files +2 −5
  1. vasild commented at 12:07 PM on October 6, 2022: contributor

    The reconciliation protocol assumes using one role consistently. Since it is irrelevant which one is which, we can imply that the initiator of the P2P connection will assume the role of reconciliation initiator.

    This protocol simplification will seep into the implementation.

  2. BIP330: drop redundant booleans from the sendtxrcncl message
    The reconciliation protocol assumes using one role consistently. Since
    it is irrelevant which one is which, we can imply that the initiator of
    the P2P connection will assume the role of reconciliation initiator.
    
    This protocol simplification will seep into the implementation.
    8b107a0af6
  3. vasild commented at 12:09 PM on October 6, 2022: contributor

    Having two booleans means we have to deal with all 4 cases:

    1. initiator=false, responder=false - meaningless
    2. initiator=false, responder=true - ok
    3. initiator=true, responder=false - ok
    4. initiator=true, responder=true - not supported (cannot be supported?)

    Allowing only 2. and 3. means we can have just one boolean and derive the value of the other from it.

    However, since it is completely irrelevant which one is which we can just assign the roles based on who is the P2P connection initiator. https://github.com/bitcoin/bitcoin/pull/23443 does that (+ trying to handle the nonsensical cases).

    I think this redundancy can be a source of confusion and/or bugs.

  4. vasild cross-referenced this on Oct 6, 2022 from issue p2p: Erlay support signaling by naumenkogs
  5. naumenkogs commented at 12:58 PM on October 6, 2022: member

    The idea behind two non-implied roles was future upgradeability, which might be not based on the connection.

    • What we will have to do then if we drop 2-booleans now? Have a SENDTXRCNCLV2, probably with the same two flags again, or another implied combination, or something completely new. We will have the same debate, although it could be expanded.
    • How likely this is to happen? I don't know. I currently have no plans to seek for better reconciliation coordination, and I don't know anyone who has.

    The fact that you caught a (comment) mistake in the code which was acked and near-merged is probably a good argument towards dropping this redundancy. @sipa what do you think?

  6. sipa commented at 1:01 PM on October 6, 2022: member

    @naumenkogs I'm fine with just making the reconciliation direction fixed by the protocol. There is still a version number in sendtxrcnl, which could be used for future extensions.

  7. naumenkogs commented at 1:05 PM on October 6, 2022: member

    Then I hope for the following events to occur shortly:

    1. Merge the Core PR
    2. Change this in a follow-up PR to Core, along with this BIP update merge

    (unless we come to a different opinion in the meanwhile) (also unless it takes too long and Luke rushes us to do something :))

  8. sipa commented at 1:10 PM on October 6, 2022: member

    I missed this was a PR already.

    ACK 8b107a0af6c0566d524041ac6c96678c88bb3b3b

  9. naumenkogs commented at 1:41 PM on October 6, 2022: member

    @vasild kinda unrelated to this PR. I'm not sure we have to always seek a synchronization between BIPs and master. I expect this BIP to be updated at least couple more times along the Erlay merging, and I think it's no big deal if e.g. the lag is one month.

  10. vasild commented at 3:58 PM on October 6, 2022: contributor

    Sounds good to use the version for future upgrades - then the sendtxrcncl can be expanded/appended with further data as needed.

    Yeah, the waterfall model where design is followed by implementation rarely works. There will be iterative steps where some mods will be done to the design while the implementation is happening. We just have to keep in mind alternative implementations and not let things diverge too much for a long time, in case somebody implements stuff based on an outdated spec.

  11. naumenkogs cross-referenced this on Oct 8, 2022 from issue p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs
  12. glozow referenced this in commit e7a0e96271 on Oct 17, 2022
  13. naumenkogs commented at 7:24 AM on October 21, 2022: member

    ACK 8b107a0af6c0566d524041ac6c96678c88bb3b3b

  14. MarcoFalke commented at 7:26 AM on October 21, 2022: member
  15. naumenkogs cross-referenced this on Oct 21, 2022 from issue p2p: Erlay support signaling follow-ups by naumenkogs
  16. luke-jr merged this on Oct 28, 2022
  17. luke-jr closed this on Oct 28, 2022

  18. fanquake referenced this in commit bcee94d107 on Nov 30, 2022
  19. sidhujag referenced this in commit 69de47c40d on Dec 1, 2022

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 18:10 UTC

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