Allow block announcements with headers #6494

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Jul 30, 2015

This is an implementation of #5982 (and based off @sipa's commit in #5307, which has been squashed into this commit).

Currently new blocks are announced via inv messages. Since headers-first download was introduced in 0.10, blocks are not processed unless the headers for the block connect to known, valid headers, so peers receiving inv messages respond with a getheaders and a getdata: the expectation is that the announcing peer will send the headers for the block and any parents unknown to the peer before delivering the new block.

In the case of a reorg, nodes currently announce only the new tip, and not the blocks that lead to the tip. This means that an extra round-trip is required to relay a reorg, because the peer receiving the inv for the tip must wait for the response to the getheaders message before being able to request the missing blocks. (For additional reasons discussed in #5307 (comment), it's not optimal to inv all the blocks added in the reorg to a headers-first peer, because this results in needless traffic due to the way getheaders requests are generated when direct-fetching inv'ed blocks.)

This pull implements a new sendheaders p2p message, which a node sends to its peers to indicate that the node prefers to receive new block announcements via a headers message rather than an inv. This implementation does a few things:

  • When announcing a new block to a peer that prefers headers, try to announce all the blocks that are being added, back to the last fork point, by sending headers for each.
  • Avoid sending headers for blocks that are already known to the peer (determined by checking pindexBestKnownBlock and pindexBestHeaderSent and their ancestors).
    • pindexBestKnownBlock is updated based on inv messages, headers sent from that peer, and getheaders messages sent from the peer.
    • pindexBestHeaderSent is a new variable that tracks the best header we have sent to the peer
  • Avoid sending more than 8 headers to the peer. This code is designed to be optimized for relaying at the tip, so a large reorg is some kind of error condition; in that case avoid spamming the peer with a lot of headers and instead fall back to the old inv mechanism.
  • If the headers to be announced aren't known to connect to headers that the peer has, then revert to sending an inv at the tip instead. This is designed to avoid DoS points from sending headers that don't connect. Since every new block should generally be announced by one peer in any pair of peers, it's expected that once headers-announcement begins on a link (ie once two nodes are known to be synced), it should be able to continue.
  • After #6148 is merged, this would allow pruning nodes to generally be able to relay reorgs to peers via headers announcements. The code that implements direct fetching upon receipt of a headers message will send getdata requests to any peer that announces a block via a headers message.

This pull includes a new python p2p test exercising the new functionality.

BIP 130 describes the proposed new sendheaders p2p message here:
https://github.com/bitcoin/bips/blob/master/bip-0130.mediawiki

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Jul 30, 2015

Contributor

This isn't really needed is it? If a node is already sending "getheaders" then presumably a node can assume it prefers headers rather than invs, perhaps?

Contributor

rebroad commented Jul 30, 2015

This isn't really needed is it? If a node is already sending "getheaders" then presumably a node can assume it prefers headers rather than invs, perhaps?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 31, 2015

Member

@rebroad Sorry, I just realized I should have emphasized more clearly that this PR also includes code to enable direct-fetching of blocks based on headers messages. Without that change, nodes that received a headers message would otherwise wait to download blocks through the existing parallel fetch mechanism, which would generally make headers-announcement inferior to inv-announcements (because we have direct-fetch code in the inv-processing logic, which means we'd be quicker to request a new block).

Consequently I think it makes sense for nodes to somehow opt-in to the new behavior. 0.10 and 0.11 nodes should continue to receive inv's even after this code is merged.

One thing that wasn't clear to me was whether it's really necessary to do the protocol version bump in order to deploy the new p2p message, given that the new message is backwards compatible -- nodes are free to ignore it. However I'm not sure I have a good grasp of how the protocol versions should be thought about.

Member

sdaftuar commented Jul 31, 2015

@rebroad Sorry, I just realized I should have emphasized more clearly that this PR also includes code to enable direct-fetching of blocks based on headers messages. Without that change, nodes that received a headers message would otherwise wait to download blocks through the existing parallel fetch mechanism, which would generally make headers-announcement inferior to inv-announcements (because we have direct-fetch code in the inv-processing logic, which means we'd be quicker to request a new block).

Consequently I think it makes sense for nodes to somehow opt-in to the new behavior. 0.10 and 0.11 nodes should continue to receive inv's even after this code is merged.

One thing that wasn't clear to me was whether it's really necessary to do the protocol version bump in order to deploy the new p2p message, given that the new message is backwards compatible -- nodes are free to ignore it. However I'm not sure I have a good grasp of how the protocol versions should be thought about.

@laanwj laanwj added the P2P label Jul 31, 2015

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Jul 31, 2015

Contributor

@sdaftuar Why not use the existing parallel fetch mechanism? I don't see any advantage in fetching the block outside of that.

Contributor

rebroad commented Jul 31, 2015

@sdaftuar Why not use the existing parallel fetch mechanism? I don't see any advantage in fetching the block outside of that.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 31, 2015

Member
Member

sipa commented Jul 31, 2015

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Aug 3, 2015

Member

Fixed @casey's nits

Member

sdaftuar commented Aug 3, 2015

Fixed @casey's nits

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Sep 9, 2015

Member

Rebased.

Member

sdaftuar commented Sep 9, 2015

Rebased.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Sep 24, 2015

Member

I'm planning to send the draft BIP out to the mailing list for comments, but before I do so, does anyone here have any guidance about whether it is necessary to bump the protocol version to introduce the new sendheaders p2p message? It is safe to ignore the message, but I'm not sure if its generally considered helpful to change the protocol version when adding even an optional message...

Member

sdaftuar commented Sep 24, 2015

I'm planning to send the draft BIP out to the mailing list for comments, but before I do so, does anyone here have any guidance about whether it is necessary to bump the protocol version to introduce the new sendheaders p2p message? It is safe to ignore the message, but I'm not sure if its generally considered helpful to change the protocol version when adding even an optional message...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 24, 2015

Member

I think it's helpful to bump the protocol version in this case. Although it's only an optional hint, it may provide more clarity (eg when documenting) and help troubleshooting.

Member

laanwj commented Sep 24, 2015

I think it's helpful to bump the protocol version in this case. Although it's only an optional hint, it may provide more clarity (eg when documenting) and help troubleshooting.

@sipa sipa referenced this pull request Sep 24, 2015

Closed

Direct headers announcement #5982

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 6, 2015

Member

How is the BIP for this coming along? Need any help?

Member

laanwj commented Oct 6, 2015

How is the BIP for this coming along? Need any help?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Oct 6, 2015

Member

@theuni suggested some alternate ideas to the "sendheaders" p2p message: either extend the version message to include a bool that indicates the preference to receive headers announcements, or even just allocate a service bit that indicates that preference.

Between those 3 options I don't feel strongly about the best way to deploy -- and in particular I can understand why it might not be great to go with adding a new p2p message that causes node state/behavior to change. @theuni mentioned he might respond on-list with his alternate suggestions; any thoughts here on those ideas?

Member

sdaftuar commented Oct 6, 2015

@theuni suggested some alternate ideas to the "sendheaders" p2p message: either extend the version message to include a bool that indicates the preference to receive headers announcements, or even just allocate a service bit that indicates that preference.

Between those 3 options I don't feel strongly about the best way to deploy -- and in particular I can understand why it might not be great to go with adding a new p2p message that causes node state/behavior to change. @theuni mentioned he might respond on-list with his alternate suggestions; any thoughts here on those ideas?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 6, 2015

Member

@sdaftuar @theuni Extending "version" continuously isn't a very scalable approach, and requires pretty strong consensus to do, as the order of entries matters. I don't really understand why we kept doing that for so long.

A service bit has the same "synchronization" problem, but does have the extra advantage of making the property searchable.

Member

sipa commented Oct 6, 2015

@sdaftuar @theuni Extending "version" continuously isn't a very scalable approach, and requires pretty strong consensus to do, as the order of entries matters. I don't really understand why we kept doing that for so long.

A service bit has the same "synchronization" problem, but does have the extra advantage of making the property searchable.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Oct 6, 2015

Member

I don't think it's important that the property is searchable. For me I'd ask should this be its own message, or should we define a "flags" message, for sending more, non-searchable capabilities.

Member

gmaxwell commented Oct 6, 2015

I don't think it's important that the property is searchable. For me I'd ask should this be its own message, or should we define a "flags" message, for sending more, non-searchable capabilities.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Oct 8, 2015

Member

@sipa @gmaxwell My preference for not sending a new messages comes from an event-driven implementor's POV. If a remote nodes are allowed to switch preferences mid-stream, it can greatly (and needlessly) complicate the local node's sending logic.

The easy way to avoid that is to disallow changing the preference back once set (this seems to be the case in @sdaftuar's BIP). Taking that a step further, it also makes sense to only accept the message early in the connection process, say directly after version/verack, and before any inv. And if that's the case, it may as well just be considered as part of the handshake.

Essentially, I would much prefer to avoid making life-of-the-connection properties stateful unless necessary. Extending the version message makes sense to me, but I understand @sipa's objection there.

@gmaxwell's suggestion seems reasonable, assuming that the "flags" message had the requirement of being sent directly after version/verack. Absence of the message would actively (though perhaps unknowingly) communicate the desire for default flags (historic behavior). Again, this just seems like an extension of the version message to me, but if changes there are deemed problematic, this would be my preference.

Member

theuni commented Oct 8, 2015

@sipa @gmaxwell My preference for not sending a new messages comes from an event-driven implementor's POV. If a remote nodes are allowed to switch preferences mid-stream, it can greatly (and needlessly) complicate the local node's sending logic.

The easy way to avoid that is to disallow changing the preference back once set (this seems to be the case in @sdaftuar's BIP). Taking that a step further, it also makes sense to only accept the message early in the connection process, say directly after version/verack, and before any inv. And if that's the case, it may as well just be considered as part of the handshake.

Essentially, I would much prefer to avoid making life-of-the-connection properties stateful unless necessary. Extending the version message makes sense to me, but I understand @sipa's objection there.

@gmaxwell's suggestion seems reasonable, assuming that the "flags" message had the requirement of being sent directly after version/verack. Absence of the message would actively (though perhaps unknowingly) communicate the desire for default flags (historic behavior). Again, this just seems like an extension of the version message to me, but if changes there are deemed problematic, this would be my preference.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Oct 16, 2015

Member

Rebased.

Member

sdaftuar commented Oct 16, 2015

Rebased.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Oct 21, 2015

Contributor

This significantly speeds up new block propagation in the normal, build-on-the-best-chain case.

Benchmark numbers from a 5-node, 4-network-hop test network I created that relays empty blocks from massachusetts to los angeles and back, twice (round-trip latency of 100 msec):

Before: 1,300 msec latency from first block-creating node sending an 'inv' to last node in the chain receiving the 'block' message

With this pull: 670 msec

So concept ACK: I haven't reviewed the code yet, and have no opinion on bumping protocol version versus new message.

Contributor

gavinandresen commented Oct 21, 2015

This significantly speeds up new block propagation in the normal, build-on-the-best-chain case.

Benchmark numbers from a 5-node, 4-network-hop test network I created that relays empty blocks from massachusetts to los angeles and back, twice (round-trip latency of 100 msec):

Before: 1,300 msec latency from first block-creating node sending an 'inv' to last node in the chain receiving the 'block' message

With this pull: 670 msec

So concept ACK: I haven't reviewed the code yet, and have no opinion on bumping protocol version versus new message.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Oct 21, 2015

Member

@gavinandresen Any chance you could repeat your test with the code in #6867? (This change is great, and we should do it, but I expect that almost all of the improvement in that benchmark will be from setting TCP_NODELAY.)

Member

gmaxwell commented Oct 21, 2015

@gavinandresen Any chance you could repeat your test with the code in #6867? (This change is great, and we should do it, but I expect that almost all of the improvement in that benchmark will be from setting TCP_NODELAY.)

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Oct 22, 2015

Contributor

Yes, I'll re-run tomorrow when I'm back in my office.

Gavin Andresen

On Oct 21, 2015, at 7:56 PM, Gregory Maxwell notifications@github.com wrote:

@gavinandresen Any chance you could repeat your test with the code in #6867? (This change is great, and we should do it, but I expect that almost all of the improvement in that benchmark will be from setting TCP_NODELAY.)


Reply to this email directly or view it on GitHub.

Contributor

gavinandresen commented Oct 22, 2015

Yes, I'll re-run tomorrow when I'm back in my office.

Gavin Andresen

On Oct 21, 2015, at 7:56 PM, Gregory Maxwell notifications@github.com wrote:

@gavinandresen Any chance you could repeat your test with the code in #6867? (This change is great, and we should do it, but I expect that almost all of the improvement in that benchmark will be from setting TCP_NODELAY.)


Reply to this email directly or view it on GitHub.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Oct 23, 2015

Member

I believe this is ready for review; so far it doesn't seem like the BIP (which relates to activation/deployment only) is likely to change substantively from what was originally proposed (see bitcoin/bips#221). It would be great to get this merged for 0.12 if possible.

Member

sdaftuar commented Oct 23, 2015

I believe this is ready for review; so far it doesn't seem like the BIP (which relates to activation/deployment only) is likely to change substantively from what was originally proposed (see bitcoin/bips#221). It would be great to get this merged for 0.12 if possible.

@gmaxwell gmaxwell added this to the 0.12.0 milestone Nov 5, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 13, 2015

Member

I don't think it's important that the property is searchable. For me I'd ask should this be its own message, or should we define a "flags" message, for sending more, non-searchable capabilities.

Late to the party, but I prefer the way how it is implemented now - to have a separate message for requesting this feature - to a generic 'flags' message, as well as to adding a version bit.

Rationale: If centralization bottlenecks can be avoided ("another set of flags to allocate"), that's strongly preferable.

utACK, intend to test.

Member

laanwj commented Nov 13, 2015

I don't think it's important that the property is searchable. For me I'd ask should this be its own message, or should we define a "flags" message, for sending more, non-searchable capabilities.

Late to the party, but I prefer the way how it is implemented now - to have a separate message for requesting this feature - to a generic 'flags' message, as well as to adding a version bit.

Rationale: If centralization bottlenecks can be avoided ("another set of flags to allocate"), that's strongly preferable.

utACK, intend to test.

src/main.cpp
@@ -4439,6 +4527,28 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
pfrom->PushMessage("getheaders", chainActive.GetLocator(pindexLast), uint256());
}
+ CNodeState *nodestate = State(pfrom->GetId());
+ // If this set of headers is valid and ends in a block with more work

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member

Can't this result in duplicate in-flights? The fact that it's not yet our tip does not imply we're not downloading it yet.

EDIT: nevermind, those aren't added to vToFetch.

@sipa

sipa Nov 13, 2015

Member

Can't this result in duplicate in-flights? The fact that it's not yet our tip does not imply we're not downloading it yet.

EDIT: nevermind, those aren't added to vToFetch.

+ assert(mi != mapBlockIndex.end());
+ CBlockIndex *pindex = mi->second;
+ if (chainActive[pindex->nHeight] != pindex) {
+ // Bail out if we reorged away from this block

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member

Do we still want to inv in this case?

@sipa

sipa Nov 13, 2015

Member

Do we still want to inv in this case?

This comment has been minimized.

@sdaftuar

sdaftuar Nov 13, 2015

Member

It's possible (probable?) that the last entry in vBlockHashesToAnnounce is our current tip (so we do want to inv it), but it is also possible that it hasn't made it there yet, and that last entry is some old tip that has now been reorged out.

I think it would be safe to add a check below to only inv the last item if it's an ancestor of our current tip; does that sound right?

@sdaftuar

sdaftuar Nov 13, 2015

Member

It's possible (probable?) that the last entry in vBlockHashesToAnnounce is our current tip (so we do want to inv it), but it is also possible that it hasn't made it there yet, and that last entry is some old tip that has now been reorged out.

I think it would be safe to add a check below to only inv the last item if it's an ancestor of our current tip; does that sound right?

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member

I think it's harmless, as the current code in this PR mimicks the old behaviour. But if we're doing the check anyway, I think we can skip the entry.

@sipa

sipa Nov 13, 2015

Member

I think it's harmless, as the current code in this PR mimicks the old behaviour. But if we're doing the check anyway, I think we can skip the entry.

This comment has been minimized.

@sdaftuar

sdaftuar Nov 13, 2015

Member

I think the code to address this edge case is easy to write, but I believe this situation is so rare that it might be better to not add this code that we can't really test? (At least I can't figure out how I'd test it...)

I think it might be better to just add a LogPrint("net", ...) debug message in the event that we're sending an inv for a block that is not on the main chain, rather than change network behavior. The downside to sending an extra inv in what we think is a rare situation is quite small, whereas if there's a bug in this code somehow and it overly suppresses inv's that could be both problematic and difficult to track down.

@sdaftuar

sdaftuar Nov 13, 2015

Member

I think the code to address this edge case is easy to write, but I believe this situation is so rare that it might be better to not add this code that we can't really test? (At least I can't figure out how I'd test it...)

I think it might be better to just add a LogPrint("net", ...) debug message in the event that we're sending an inv for a block that is not on the main chain, rather than change network behavior. The downside to sending an extra inv in what we think is a rare situation is quite small, whereas if there's a bug in this code somehow and it overly suppresses inv's that could be both problematic and difficult to track down.

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member

Fair enough. It won't hurt.

@sipa

sipa Nov 13, 2015

Member

Fair enough. It won't hurt.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 13, 2015

Member

Concept ACK. Thorough code review ACK with one small nit. Lightly tested (two peers with this pull succesfully relay blocks across the internet, verified with -debug=net).

Member

sipa commented Nov 13, 2015

Concept ACK. Thorough code review ACK with one small nit. Lightly tested (two peers with this pull succesfully relay blocks across the internet, verified with -debug=net).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 13, 2015

Member

Needs rebase.

Member

sipa commented Nov 13, 2015

Needs rebase.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 13, 2015

Member

Rebased and added log message when trying to announce a stale block.

Member

sdaftuar commented Nov 13, 2015

Rebased and added log message when trying to announce a stale block.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 16, 2015

Member

I'm looking into an issue @morcos noticed this afternoon; it seems problematic to update pindexBestKnownBlock from a locator received with a getheaders message, because that would imply we can download such blocks from our peer. Yet our peer can generate locators from headers they have rather than from blocks they have.

I am testing just removing that block of code; will update this PR once I confirm that is safe.

Member

sdaftuar commented Nov 16, 2015

I'm looking into an issue @morcos noticed this afternoon; it seems problematic to update pindexBestKnownBlock from a locator received with a getheaders message, because that would imply we can download such blocks from our peer. Yet our peer can generate locators from headers they have rather than from blocks they have.

I am testing just removing that block of code; will update this PR once I confirm that is safe.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 16, 2015

Member

@sdaftuar @morcos Good catch, and too bad. I doubt it matters much.

Member

sipa commented Nov 16, 2015

@sdaftuar @morcos Good catch, and too bad. I doubt it matters much.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 17, 2015

Member

Updated this PR to eliminate updating pindexBestKnownBlock from the locator, and squashed back down to one commit.

The reason I had put this in initially was that I was concerned about there being a potential bootstrapping problem, but after further thought and some light testing I don't think there's a problem. The initial getheaders sync that happens after a connection is established should ensure that headers announcements start flowing immediately.

Member

sdaftuar commented Nov 17, 2015

Updated this PR to eliminate updating pindexBestKnownBlock from the locator, and squashed back down to one commit.

The reason I had put this in initially was that I was concerned about there being a potential bootstrapping problem, but after further thought and some light testing I don't think there's a problem. The initial getheaders sync that happens after a connection is established should ensure that headers announcements start flowing immediately.

src/main.cpp
BOOST_FOREACH(const CBlockHeader& header, headers) {
CValidationState state;
if (pindexLast != NULL && header.hashPrevBlock != pindexLast->GetBlockHash()) {
Misbehaving(pfrom->GetId(), 20);
return error("non-continuous headers sequence");
}
+ BlockMap::iterator it = mapBlockIndex.find(header.GetHash());
+ if (it != mapBlockIndex.end()) {

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

It might be better to delete this check. pindexLast shouldn't be set to a block that returns false from AcceptBlockHeader even if we already have the header.

@morcos

morcos Nov 17, 2015

Member

It might be better to delete this check. pindexLast shouldn't be set to a block that returns false from AcceptBlockHeader even if we already have the header.

src/main.cpp
@@ -4515,12 +4574,21 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
}
CBlockIndex *pindexLast = NULL;
+ std::vector<CBlockIndex *> vToFetch;
+ bool fCanDirectFetch = CanDirectFetch(chainparams.GetConsensus());

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

add !fimporting && !fReindex

@morcos

morcos Nov 17, 2015

Member

add !fimporting && !fReindex

This comment has been minimized.

@sdaftuar

sdaftuar Nov 17, 2015

Member

This block of code already has a guard so that we don't process headers received while importing/reindexing.

@sdaftuar

sdaftuar Nov 17, 2015

Member

This block of code already has a guard so that we don't process headers received while importing/reindexing.

src/main.cpp
+ CNodeState *nodestate = State(pfrom->GetId());
+ // If this set of headers is valid and ends in a block with more work
+ // than our tip, download as much as possible.
+ if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && chainActive.Tip()->nChainWork < pindexLast->nChainWork) {

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

nit: already checked fCanDirectFetch above. maybe check vToFetch.size()

@morcos

morcos Nov 17, 2015

Member

nit: already checked fCanDirectFetch above. maybe check vToFetch.size()

+ send getdata for the block [expect: block]
+b. node mines another block [expect: inv]
+ send getheaders and getdata [expect: headers, then block]
+c. node mines another block [expect: inv]

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

comment incorrect

@morcos

morcos Nov 17, 2015

Member

comment incorrect

qa/rpc-tests/sendheaders.py
+ # commence and keep working.
+ test_node.send_message(msg_sendheaders())
+ prev_tip = int("0x" + self.nodes[0].getbestblockhash() + "L", 0)
+ test_node.get_headers(locator=[prev_tip], hashstop=0L)

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

this shouldn't be necessary

@morcos

morcos Nov 17, 2015

Member

this shouldn't be necessary

This comment has been minimized.

@sdaftuar

sdaftuar Nov 17, 2015

Member

Surprisingly, this code is necessary to make the test work. Turns out there's an unusual feature in the getheaders handling, where we set pindexBestHeaderSent to chainActive.Tip() in two situations: if we actually are sending the tip, or if we have nothing to send. I think the latter case can only happen if our peer has our tip in their locator, so I believe this behavior is desirable, if surprising.

I've added a comment to the relevant line of code in main.cpp explaining this behavior.

@sdaftuar

sdaftuar Nov 17, 2015

Member

Surprisingly, this code is necessary to make the test work. Turns out there's an unusual feature in the getheaders handling, where we set pindexBestHeaderSent to chainActive.Tip() in two situations: if we actually are sending the tip, or if we have nothing to send. I think the latter case can only happen if our peer has our tip in their locator, so I believe this behavior is desirable, if surprising.

I've added a comment to the relevant line of code in main.cpp explaining this behavior.

qa/rpc-tests/sendheaders.py
+ tip = self.mine_blocks(1)
+ assert_equal(inv_node.check_last_announcement(inv=[tip]), True)
+ assert_equal(test_node.check_last_announcement(headers=[tip]), True)
+ test_node.get_data([tip])

This comment has been minimized.

@morcos

morcos Nov 17, 2015

Member

why do you do this?

@morcos

morcos Nov 17, 2015

Member

why do you do this?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 18, 2015

Member

Pushed a commit to address @morcos' comments.

In cleaning up the RPC test I was surprised with what happens to pindexBestHeaderSent when a peer sends a getheaders with a locator that is up-to-date with our tip. Though I took out the code to parse the locator and update pindexBestKnownBlock, it turns out that if we generate no headers to return then we will end up setting pindexBestHeaderSent to equal chainActive.Tip().

This behavior was just due to a quirk in the code (the test that does that is checking to see if we've walked off the end of chainActive and assumes that if we're at a NULL pointer then we must have returned our tip to the peer), but I believe this is correct behavior. I added a comment to the code explaining this quirk, but am leaving it in because I think it's correct and beneficial.

On a separate note: I was reminded when I looked at this again that if a peer sends us a getheaders with a locator that is caught up to our tip, then we'll send back an empty headers message. Is it important that this behavior be preserved? I left it unchanged for now, but I can clean this up and suppress an empty response if it's safe to do so, either in this pull or separately.

Member

sdaftuar commented Nov 18, 2015

Pushed a commit to address @morcos' comments.

In cleaning up the RPC test I was surprised with what happens to pindexBestHeaderSent when a peer sends a getheaders with a locator that is up-to-date with our tip. Though I took out the code to parse the locator and update pindexBestKnownBlock, it turns out that if we generate no headers to return then we will end up setting pindexBestHeaderSent to equal chainActive.Tip().

This behavior was just due to a quirk in the code (the test that does that is checking to see if we've walked off the end of chainActive and assumes that if we're at a NULL pointer then we must have returned our tip to the peer), but I believe this is correct behavior. I added a comment to the code explaining this quirk, but am leaving it in because I think it's correct and beneficial.

On a separate note: I was reminded when I looked at this again that if a peer sends us a getheaders with a locator that is caught up to our tip, then we'll send back an empty headers message. Is it important that this behavior be preserved? I left it unchanged for now, but I can clean this up and suppress an empty response if it's safe to do so, either in this pull or separately.

Allow block announcements with headers
This replaces using inv messages to announce new blocks, when a peer requests
(via the new "sendheaders" message) that blocks be announced with headers
instead of inv's.

Since headers-first was introduced, peers send getheaders messages in response
to an inv, which requires generating a block locator that is large compared to
the size of the header being requested, and requires an extra round-trip before
a reorg can be relayed.  Save time by tracking headers that a peer is likely to
know about, and send a headers chain that would connect to a peer's known
headers, unless the chain would be too big, in which case we revert to sending
an inv instead.

Based off of @sipa's commit to announce all blocks in a reorg via inv,
which has been squashed into this commit.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 23, 2015

Member

I discovered there were some problems in the direct fetch logic.

The code was structured so that we could only direct fetch blocks which were announced in a set of headers. However, in the case of a reorg, we may have some of the headers on the reorged-to-chain, which our peers will not re-announce to us -- meaning that the direct-fetch logic wouldn't request needed missing blocks immediately.

I've restructured the direct fetch logic so that if we want to download towards the latest block a peer has announced to us, then we walk back from that tip and consider every block until we find one that is on our current chain. (We try to download every block that we don't have that is not already in-flight, and we bail out if we're walking back too many blocks.)

I also realized that sendheaders.py had no tests for the direct fetch logic, so I updated it to exercise that part of the code as well.

Member

sdaftuar commented Nov 23, 2015

I discovered there were some problems in the direct fetch logic.

The code was structured so that we could only direct fetch blocks which were announced in a set of headers. However, in the case of a reorg, we may have some of the headers on the reorged-to-chain, which our peers will not re-announce to us -- meaning that the direct-fetch logic wouldn't request needed missing blocks immediately.

I've restructured the direct fetch logic so that if we want to download towards the latest block a peer has announced to us, then we walk back from that tip and consider every block until we find one that is on our current chain. (We try to download every block that we don't have that is not already in-flight, and we bail out if we're walking back too many blocks.)

I also realized that sendheaders.py had no tests for the direct fetch logic, so I updated it to exercise that part of the code as well.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 24, 2015

Contributor

Great work @sdaftuar

Contributor

dcousens commented Nov 24, 2015

Great work @sdaftuar

@@ -4521,6 +4584,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
Misbehaving(pfrom->GetId(), 20);
return error("non-continuous headers sequence");
}
+ BlockMap::iterator it = mapBlockIndex.find(header.GetHash());

This comment has been minimized.

@sdaftuar

sdaftuar Nov 24, 2015

Member

Looks like I forgot to remove this.

@sdaftuar

sdaftuar Nov 24, 2015

Member

Looks like I forgot to remove this.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 28, 2015

Member

Needs rebase.

Member

sipa commented Nov 28, 2015

Needs rebase.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 29, 2015

Member

Closed via #7129.

Member

sipa commented Nov 29, 2015

Closed via #7129.

@sipa sipa closed this Nov 29, 2015

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Dec 7, 2016

Direct header announcements (BIP130)
This work is based on @sdaftuar work in Bitcoin Core PR #6494 (9fa8c48).
This work is based on @dgenr8 cherry picks in Bitcoin XT #137

This implements BIP130. This implementation is a refactor/rewrite on
previous work done in Bitcoin Core.

This work supports headersfirst together with thin blocks. It is refactored to support unittests,
and unittests have been written for most of the logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment