Support for compact blocks together with segwit

sipa commented at 3:22 pm on July 22, 2016: member

See https://github.com/bitcoin/bips/pull/423 for BIP changes.

instagibbs commented at 4:45 pm on July 22, 2016: member

nit: Might be worthwhile to print out the cmpctblk version being reconstructed in debug log

nit aside, utACK https://github.com/bitcoin/bitcoin/pull/8393/commits/1b417c20a6b0205a98268f95e6e19d27def076e9

btcdrak commented at 4:48 pm on July 22, 2016: contributor

Tested ACK 1b417c2 (I tested this branch extensively before this PR was opened).

jonasschnelli added the label P2P on Jul 23, 2016

NicolasDorier commented at 4:50 am on July 24, 2016: contributor

utACK 1b417c2 I will write some tests with NBitcoin soon.

in src/main.cpp: in 1b417c20a6 outdated

478@@ -476,16 +479,16 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
479 }
480 
481 void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pfrom) {
482-    if (nLocalServices & NODE_WITNESS) {
483-        // Don't ever request compact blocks when segwit is enabled.
484+    if (nLocalServices & ~pfrom->nServices & NODE_WITNESS) {
485+        // Never ask from peers who can't provide witnesses.

sdaftuar commented at 1:12 pm on July 25, 2016:

This means that prior to segwit activation, blocks found by unupgraded miners will propagate without the benefit of compactblock announcements to upgraded nodes.

Perhaps it’d be better to instead use the BIP9 LOCKED_IN window to transition from receiving compactblock announcements from any peer regardless of nServices, towards NODE_WITNESS peers, and once ACTIVE state is reached, we enforce that all announcing peers are NODE_WITNESS?

To be concrete, here’s one suggestion:

before LOCKED_IN, use existing algorithm
after LOCKED_IN, don’t allow NODE_WITNESS peers to be evicted by non-WITNESS peers, and preferentially first evict non-WITNESS peers when trying to add a WITNESS peer
after ACTIVATED, evict any non-WITNESS peers we have left making announcements.

I think this ought to provide a smooth transition, but I actually am not sure we even need that second step, we can probably be a little more disruptive around the LOCKED_IN time to ensure that we’re ready at activation.

sipa commented at 11:37 pm on July 25, 2016: member

Reminder for myself: adapt to use the suggestion from BIP152 to just send both sendcmpct version 1 and version 2 (and let the 2 be ignored by pre-witness peers), rather than trying to guess what the peer will want based on service flags. Suggested by @TheBlueMatt

sipa force-pushed on Jul 29, 2016

sipa commented at 1:34 am on July 30, 2016: member

Implemented the suggestion above.

sipa force-pushed on Jul 30, 2016

sipa commented at 6:22 pm on July 30, 2016: member

Fixed a bug found by @sdaftuar (see #8418).

jl2012 commented at 10:36 am on August 4, 2016: contributor

need backport to 0.13

MarcoFalke added the label Needs backport on Aug 4, 2016

in src/main.cpp: in 8b3343ffc8 outdated

5186@@ -5183,7 +5187,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5187             LOCK(cs_main);
5188             State(pfrom->GetId())->fProvidesHeaderAndIDs = !(nLocalServices & NODE_WITNESS) || (nCMPCTBLOCKVersion == 2);
5189             State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
5190-            State(pfrom->GetId())->fWantCmpctWitness = (nCMPCTBLOCKVersion == 2);
5191+            State(pfrom->GetId())->fSupportsCmpctWitness = (nCMPCTBLOCKVersion == 2);

sdaftuar commented at 5:19 pm on August 4, 2016:

This means that we’d overwrite fSupportsCmpctWitness if our peer sends a version 2 message followed by a version 1 message. If we intend to require a certain ordering of sendcmpct announcements, I think that should be more clearly called out in the BIP (I’ll leave a separate comment in bitcoin/bips#423 to indicate the language I suggest improving). But my initial reaction is that we shouldn’t impose this constraint.

Anyway I’m a bit confused on the interaction here between NODE_WITNESS and how we process sendcmpct messages. Per BIP 152, if we don’t support version 2 compact blocks, we must ignore version 2 sendcmpct messages. But this code doesn’t appear to be written this way (ie this processes version 2 sendcmpct messages, even if NODE_WITNESS is off).

In particular, it seems that this code could break if eg it was part of a release (like 0.13.0) where NODE_WITNESS would not be set, but at some point in the future it had a peer that supported only version 2 compact blocks (as we’d process the version 2 message and think we could communicate with the peer, even though we can’t).

sipa commented at 5:34 pm on August 4, 2016:

@sdaftuar I think they have to be sent in increasing order, so that the receiver can use the simple “the last understood sendcmpct counts” logic.

You are right that we should ignore v2 sendcmpct messages when we don’t have NODE_WITNESS. That would make the behaviour of a non-defined segwit node identical to a pre-segwit node. Agree?

sdaftuar commented at 5:49 pm on August 4, 2016:

@sipa I guess that’s fine. I noted the language I thought could be clarified to make this more clear in the BIP pr.

I agree that ignoring v2 messages if we’re not NODE_WITNESS should result in the identical behavior we’re looking for.

sipa force-pushed on Aug 5, 2016

sipa added this to the milestone 0.13.0 on Aug 6, 2016

sipa commented at 0:00 am on August 6, 2016: member

@sdaftuar Updated.

sipa commented at 9:46 pm on August 6, 2016: member

Tested:

mainnet (this PR) syncing from mainnet (this PR)
testnet (this PR) syncing from testnet (this PR)
testnet (this PR, with segwit activation disabled) syncing from testnet (this PR)
testnet (post-CB, pre-segwit code) syncing from testnet (this PR)

They all seem to be using compact blocks for transfer.

instagibbs commented at 8:36 pm on August 8, 2016: member

re-utACK https://github.com/bitcoin/bitcoin/pull/8393/commits/00fabfeca09659f620b773ea5627c9a8ff4443cb

laanwj commented at 7:03 am on August 10, 2016: member

backport: this is for 0.13.1 I suppose?

sipa commented at 7:08 am on August 10, 2016: member

Yes, no need for this in 0.13.0.

sipa added this to the milestone 0.13.1 on Aug 10, 2016

sipa removed this from the milestone 0.13.0 on Aug 10, 2016

sipa added this to the milestone 0.13.0 on Aug 10, 2016

sipa removed this from the milestone 0.13.1 on Aug 10, 2016

sipa added this to the milestone 0.13.1 on Aug 10, 2016

sipa removed this from the milestone 0.13.0 on Aug 10, 2016

sipa removed this from the milestone 0.13.1 on Aug 10, 2016

sipa added this to the milestone 0.13.1 on Aug 10, 2016

sdaftuar commented at 2:59 pm on August 18, 2016: member

@sipa Thoughts on this comment: #8393 (review) ?

sipa commented at 10:56 am on August 25, 2016: member

@sdaftuar Unsure… I think it’s easier and more transparent to see failure/relay problems as early as possible, and rather at upgrade time than at lockedin time.

Also, relay should work fine (and use compact blocks) as soon as a block from an unupgraded miner enters the network of upgraded peers, right? I assume that things like the relay network will even help propagation across that “boundary” to occur.

in src/main.cpp: in 00fabfeca0 outdated

5098             // We send this to non-NODE NETWORK peers as well, because
5099             // they may wish to request compact blocks from us
5100             bool fAnnounceUsingCMPCTBLOCK = false;
5101             uint64_t nCMPCTBLOCKVersion = 1;
5102             pfrom->PushMessage(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);
5103+            if (nLocalServices & NODE_WITNESS) {

rebroad commented at 1:50 pm on August 26, 2016:

Thanks. I find this much more readable and understandable than when developers create a function to do this. e.g. IsWitnessEnabled() !

sipa commented at 1:52 pm on August 26, 2016:

It is not the same. This condition is about whether a segwit softfork is defined, not whether it has activated.

TheBlueMatt commented at 9:14 pm on September 5, 2016: member

I proposed more specifics on version negotiation at https://github.com/bitcoin/bips/pull/423#issuecomment-244811877 which I believe might require slight implementation tweaks, though nothing significant.

sipa force-pushed on Sep 6, 2016

sipa commented at 11:34 am on September 6, 2016: member

Added a commit that introduces the ping-pong based waiting described by the latest https://github.com/bitcoin/bips/pull/423.

cc @TheBlueMatt

TheBlueMatt commented at 4:04 pm on September 6, 2016: member

I dont think you need the ping? As indicated in the new BIP text, you can know that the peer will respond to your request for a compact block with /at least/ a compact block of version X after you have seen at least that version in one of their announcements. For us, this is sufficient as we only care about them responding with version 2.

sdaftuar commented at 1:49 pm on September 7, 2016: member

Also, relay should work fine (and use compact blocks) as soon as a block from an unupgraded miner enters the network of upgraded peers, right? I assume that things like the relay network will even help propagation across that “boundary” to occur. @sipa That sounds right to me. I was thinking that the first miners to upgrade might be at a disadvantage (because they would stop receiving blocks from old miners as quickly as their peers), but I think you’re right that the relay network should mitigate that.

sipa commented at 11:10 am on September 8, 2016: member

@TheBlueMatt I believe you’re right, and the ping isn’t necessary… but thinking about this makes my head hurt, so I feel better with an actual synchronization in place - even if we only need it later.

TheBlueMatt commented at 2:43 pm on September 9, 2016: member

I removed the ping at https://github.com/TheBlueMatt/bitcoin/tree/segwitcb . While the proposed BIP text is pretty dense and confusing, the implementation is pretty straightforward: if they have sent us a sendcmpct version 2, we know they will encode things as version 2, so we gate actually requesting any compact blocks on having seen such a message. We also keep a boolean for which version they requested we send them.

TheBlueMatt commented at 7:10 pm on September 13, 2016: member

Simplifed the impementation at the branch linked above and added a comment or two after @sdaftuar and @morcos looked over it a bit.

jonasschnelli commented at 7:12 pm on September 13, 2016: contributor

Needs rebase.

sipa force-pushed on Sep 13, 2016

sipa commented at 9:04 pm on September 13, 2016: member

Reviewed and included @TheBlueMatt’s suggested commits, and rebased. I have not tested this, and I likely won’t have time for that this week.

TheBlueMatt commented at 9:29 pm on September 13, 2016: member

Feel free to squash commits before release (you almost certainly want this since the different commits implement a different protocol)

sipa commented at 9:30 pm on September 13, 2016: member

@TheBlueMatt Absolutely, I was planning to, especially as some commits revert changes from earlier ones.

sdaftuar commented at 0:54 am on September 15, 2016: member

Code review ACK. I’m working on updating the p2p test framework and p2p-compactblocks.py test to exercise the new behavior.

(Unfortunately I just discovered that one of the existing tests in p2p-compactblocks.py is pretty broken, working on a fix.)

in src/main.cpp: in f12b4f7ae8 outdated

5221-            State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
5222+            // fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
5223+            if (!State(pfrom->GetId())->fProvidesHeaderAndIDs) {
5224+                State(pfrom->GetId())->fProvidesHeaderAndIDs = true;
5225+                State(pfrom->GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
5226+                State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;

sdaftuar commented at 1:17 pm on September 16, 2016:

This prevents changing whether we announce to this peer. It should be moved out of this if() condition, but I’m not sure yet what the condition should be for updating fPreferHeaderAndIDs.

sipa commented at 11:24 am on September 19, 2016:

Cherry-picked the commit from https://github.com/TheBlueMatt/bitcoin/commits/segwitcb.

sipa force-pushed on Sep 19, 2016

in src/main.cpp: in 2048b69df3 outdated

477@@ -465,16 +478,16 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
478 }
479 
480 void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pfrom, CConnman& connman) {
481-    if (pfrom->GetLocalServices() & NODE_WITNESS) {
482-        // Don't ever request compact blocks when segwit is enabled.
483+    if (!nodestate->fSupportsDesiredCmpctVersion) {

instagibbs commented at 7:33 pm on September 20, 2016:

This condition is already checked before calling this function. Further it isn’t clear how this stops our node from asking non-witness nodes to pre-emptively send cmpctblocks once segwit is activated.

TheBlueMatt commented at 8:28 pm on September 21, 2016:

A node is never prevented from chosing to send us a message, but later in this function is the only place we send a sendcmpct with announce set to true, so no matter how you call this it should be safe (note that we set fSupportsDesiredCmpctVersion based on NODE_WITNESS, not based on current activation status).

As for the duplicate check…whatever, up to the PR owner, I suppose :p.

instagibbs commented at 9:23 pm on September 21, 2016:

I failed to understand that pfrom->nLocalServices is talking about your own services. Nevermind.

sdaftuar commented at 11:45 pm on September 20, 2016: member

What’s the correct behavior if an old peer sends us a compact block message that is unrequested? If I understand right, we’re not currently checking to confirm that cmpctblock messages are only coming from peers with whom we’ve negotiated our preferred compact block version.

Accepting the header seems safe enough, and it’s possible for the block to reconstruct correctly (eg if the block has no witness commitment), so there’s perhaps no harm in trying to reconstruct, but it’s probably not worth sending a GETBLOCKTXN message for missing transactions if the peer isn’t using the preferred protocol version?

TheBlueMatt commented at 8:29 pm on September 21, 2016: member

@sdaftuar I think we should emulate the block logic - if a peer sends us a block that is non-witness it should be the same case as if they send us a compact block message when we have negotiated to receive version 1, as that implies that block is a non-witness compact block.

Alternatively, we could ban them…I suppose I dont care either way.

in src/main.cpp: in 2048b69df3 outdated

297     //! Whether this peer can give us witnesses
298     bool fHaveWitness;
299+    //! Whether this peer wants witnesses in cmpctblocks/blocktxns
300+    bool fWantsCmpctWitness;
301+    /**
302+     * For witness nodes: whether this peer sends witnesses in cmpctblocks/blocktxns

instagibbs commented at 1:32 pm on September 22, 2016:

Maybe add “local” in both spots?

For local witness nodes

Semi ambiguous as-is.

TheBlueMatt commented at 2:10 pm on September 22, 2016:

local also seems unclear to me, how about “If we’ve announced NODE_WITNESS to this peer:” etc?

instagibbs commented at 2:13 pm on September 22, 2016:

ACK

instagibbs commented at 2:07 pm on September 22, 2016: member

nit aside code review ACK https://github.com/bitcoin/bitcoin/pull/8393/commits/2048b69df36a5d1f49db2de6e099209631c1f490

sdaftuar commented at 6:06 pm on September 22, 2016: member

ACK

If you’d like to address the minor issue I noted above about processing a compact block from a v1 peer post-segwit activation, I have a commit that will just do headers processing in such a case: https://github.com/sdaftuar/bitcoin/commit/f6a86fb2112a4e7341bea6aaec0d4e55829e49c9

Also, I’ve reworked the p2p-compactblocks test for this PR. The branch I’ve been working on is here: https://github.com/sdaftuar/bitcoin/tree/test-8393-5. If you want to include the tests that I wrote here from my branch, please feel free to take a look. Note that one of the commits (https://github.com/sdaftuar/bitcoin/commit/e049ebf51fc3b8ab4f7a2f4ce19d8698ecf467bf) is a separate but necessary bugfix that I discovered in mininode.py, relating to witness deserialization.

Finally, the last two commits in that branch (https://github.com/sdaftuar/bitcoin/commit/d990722b56f02f3272c49eeef4c952992bd9efaa and https://github.com/sdaftuar/bitcoin/commit/2419b6b0256cff6ff9de1293e0cb87e2537e0f7f) may be of interest: I noticed it was a very small change to allow downloading compact blocks from old peers before segwit-activation (even if we don’t ever ask such peers to announce compact blocks to us), so I coded that up and added a test for that case as well.

laanwj removed this from the milestone 0.13.1 on Sep 22, 2016

jonasschnelli added this to the milestone 0.13.1 on Sep 22, 2016

TheBlueMatt commented at 8:23 pm on September 22, 2016: member

I’m in favor of fixing the download-from-0.13 nodes bug given that its such a small difference.

instagibbs commented at 10:17 pm on September 26, 2016: member

@sdaftuar I’d ACK https://github.com/sdaftuar/bitcoin/commit/f6a86fb2112a4e7341bea6aaec0d4e55829e49c9 but I’m not sure the other changes are needed as those seem like unlikely corner cases to me that might happen just once and never again.

sipa commented at 3:13 am on September 28, 2016: member

I addressed @instagibbs’s nits and cherry-picked @sdaftuar’s proposed commits (I didn’t review the tests yet). I’ll think about the last two commits.

sipa commented at 2:01 pm on September 28, 2016: member

I’d rather not pepper the block fetching code further with edge cases to deal with non-witness to witness propagation. The network should work fine without that, or we have other problems - and we’ll know far in advance anyway.

Let me know if this should be squashed.

sipa force-pushed on Sep 28, 2016

sipa commented at 2:22 pm on September 28, 2016: member

Squashed version: https://github.com/sipa/bitcoin/commits/segwitcb_squashed

in src/main.cpp: in 29f4582847 outdated

5689@@ -5690,6 +5690,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5690 
5691         CNodeState *nodestate = State(pfrom->GetId());
5692 
5693+        if (IsWitnessEnabled(pindex, chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) {

sdaftuar commented at 7:01 pm on September 28, 2016:

Oops. This should actually be pindex->prev I think? Sorry…

instagibbs commented at 8:40 pm on September 28, 2016:

pprev* ? :)

sdaftuar commented at 8:13 pm on September 30, 2016: member

ACK after fixing the minor issue in my bugfix for the minor issue (sorry). Feel free to squash.

sipa force-pushed on Oct 2, 2016

sipa commented at 11:42 pm on October 2, 2016: member

Nit addressed and squashed.

gmaxwell commented at 5:40 am on October 3, 2016: contributor

ACK

in src/main.cpp: in 620ec326cf outdated

5790@@ -5757,7 +5791,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5791         } else if (status == READ_STATUS_FAILED) {
5792             // Might have collided, fall back to getdata now :(
5793             std::vector<CInv> invs;
5794-            invs.push_back(CInv(MSG_BLOCK, resp.blockhash));
5795+            invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom, chainActive.Tip(), chainparams.GetConsensus()), resp.blockhash));

TheBlueMatt commented at 2:39 pm on October 3, 2016:

I believe this is wrong. If a witness peer announces, directly to us, via compact blocks, the first segwit block, and we are missing the second-to-first segwit block (or its currently in-flight) when we receive the response to this message, we will ban the peer sending the first segwit block as it will respond in non-witness form, because of this request.

sipa commented at 1:29 pm on October 4, 2016:

Does this still need changing, if we include #8871?

TheBlueMatt commented at 1:48 pm on October 4, 2016:

No, I believe #8871 will be sufficient to fix this issue.

TheBlueMatt commented at 2:40 pm on October 3, 2016: member

There are a few places where we gate things on only fSupportsDesiredCmpctVersion, and not (LocalServices() & NODE_WITNESS) && fSupportsDesiredCmpctVersion (my fault, I wrote that), while this is still correct (since (LocalServices() & NODE_WITNESS) is a global constant after startup), it is somewhat strange and would break if we had to change something such that (LocalServices() & NODE_WITNESS) were no longer globally constant. Don’t really think its worth fixing, just thought it was worth a mention.

TheBlueMatt commented at 1:53 pm on October 4, 2016: member

With #8871 included first, utACK.

Make GetFetchFlags always request witness objects from witness peers

This fixes a bug where we might (in exceedingly rare circumstances)
accidentally ban a node for sending us the first (potentially few)
segwit blocks in non-segwit mode.

06128da751

Fix overly-prescriptive p2p-segwit test for new fetch logic be7555f0c0

sipa force-pushed on Oct 4, 2016

sipa commented at 4:44 pm on October 4, 2016: member

Rebased on top of #8871.

Use cmpctblock type 2 for segwit-enabled transfer

Contains version negotiation logic by Matt Corallo and bugfixes by
Suhas Daftuar.

6aa28abf53

[qa] Fix bug in mininode witness deserialization

Also improve tx printing

f5b9b8f437

[qa] Add support for compactblocks v2 to mininode 422fac649f

[qa] Update p2p-compactblocks.py for compactblocks v2 27acfc1d2e

sipa force-pushed on Oct 4, 2016

sdaftuar commented at 5:34 pm on October 4, 2016: member

ACK

petertodd commented at 8:24 am on October 10, 2016: contributor

utACK https://github.com/bitcoin/bitcoin/pull/8393/commits/f5b9b8f437c040205896ad0d7a6656efa08b5601

jonasschnelli commented at 10:46 am on October 10, 2016: contributor

utACK https://github.com/bitcoin/bitcoin/pull/8393/commits/f5b9b8f437c040205896ad0d7a6656efa08b5601 https://bitcoin.jonasschnelli.ch/pulls/8393/

sipa merged this on Oct 10, 2016

sipa closed this on Oct 10, 2016

sipa referenced this in commit 6429cfa8a7 on Oct 10, 2016

btcdrak commented at 12:05 pm on October 10, 2016: contributor

Post humus, re-tACK f5b9b8f437c040205896ad0d7a6656efa08b5601

laanwj referenced this in commit e47299a8f2 on Oct 13, 2016

laanwj referenced this in commit 61e282b62d on Oct 13, 2016

laanwj referenced this in commit 611cc5096e on Oct 13, 2016

laanwj referenced this in commit fe1975a974 on Oct 13, 2016

laanwj referenced this in commit 890ac25638 on Oct 13, 2016

laanwj referenced this in commit 4bb9ce8a95 on Oct 13, 2016

laanwj removed the label Needs backport on Oct 13, 2016

in src/main.cpp: in 27acfc1d2e

5152-            uint64_t nCMPCTBLOCKVersion = 1;
5153+            uint64_t nCMPCTBLOCKVersion = 2;
5154+            if (pfrom->GetLocalServices() & NODE_WITNESS)
5155+                pfrom->PushMessage(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);
5156+            nCMPCTBLOCKVersion = 1;
5157             pfrom->PushMessage(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);

rebroad commented at 8:42 am on October 28, 2016:

Seems a bit of a strange design that it needs to send two SENDCMPCT messages at once…. can it not send one OR the other instead?

gladcow referenced this in commit 037097d353 on Mar 2, 2018

gladcow referenced this in commit 0dfa52f5f1 on Mar 13, 2018

gladcow referenced this in commit 9f97bcdfe4 on Mar 14, 2018

gladcow referenced this in commit 8e448bc11d on Mar 15, 2018

gladcow referenced this in commit 1705db873c on Mar 15, 2018

gladcow referenced this in commit 9538881cbb on Mar 15, 2018

gladcow referenced this in commit 2211443ec9 on Mar 24, 2018

gladcow referenced this in commit f4b6ddbdc8 on Apr 4, 2018

UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018

andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019

CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019

DrahtBot locked this on Dec 16, 2021

Support for compact blocks together with segwit #8393