Combine expedited block and xthin block handling #509
Conversation
| @@ -5884,13 +5884,13 @@ bool ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vRecv, in | |||
| else if (strCommand == NetMsgType::XPEDITEDBLK) | |||
| { | |||
| // ignore the expedited message unless we are at the chain tip... | |||
| if (!fImporting && !fReindex && IsChainNearlySyncd()) | |||
| if (!fImporting && !fReindex && !IsInitialBlockDownload() && IsThinBlocksEnabled()) | |||
There was a problem hiding this comment.
For reviewers - I changed this because it's now the same condition as the xthin block code path. I'm unsure which one is preferable but am fairly sure they should be the same.
|
Rebased too given the conflict. Now just on top of #502 |
|
the failure is due to auto check on formatting for `thinblock.{cpp,h}, the diff started here: https://travis-ci.org/BitcoinUnlimited/BitcoinUnlimited/jobs/228529744#L1177 the error is raised here: see https://travis-ci.org/BitcoinUnlimited/BitcoinUnlimited/jobs/228529744#L1244 all the other tests pass. edit: I'm able to reproduce the result of travis formatting checking on locally puled version of the repo. (i.e. |
|
@kyuupichan, @ftrader and I set up a 3 nodes Xpedited sub net and tested this set up and we verified that the patch achieve the goal of preserving the functionality while at the same time reorganizing the code in a more rational way. |
src/thinblock.cpp
Outdated
| { | ||
| Misbehaving(pfrom->GetId(), 10); | ||
| return error("%s from peer %s (%d) will not connect, unknown previous block %s", | ||
| strCommand, pfrom->addrName.c_str(), pfrom->id, prevHash.ToString()); |
There was a problem hiding this comment.
this one of the site blamed to be not properly formatted by clang-format-3.8, this is the actual diff:
--- thinblock.cpp
+++ thinblock.cpp_formatted
@@ -277,8 +277,8 @@
if (mi == mapBlockIndex.end())
{
Misbehaving(pfrom->GetId(), 10);
- return error("%s from peer %s (%d) will not connect, unknown previous block %s",
- strCommand, pfrom->addrName.c_str(), pfrom->id, prevHash.ToString());
+ return error("%s from peer %s (%d) will not connect, unknown previous block %s", strCommand,
+ pfrom->addrName.c_str(), pfrom->id, prevHash.ToString());There was a problem hiding this comment.
Thanks, I'll "fix" the formatting.
src/thinblock.cpp
Outdated
| // Thin block does not fit within our blockchain | ||
| Misbehaving(pfrom->GetId(), 100); | ||
| return error("%s from peer %s (%d) contextual error: %s", strCommand, pfrom->addrName.c_str(), | ||
| pfrom->id, state.GetRejectReason().c_str()); |
src/thinblock.cpp
Outdated
| else | ||
| { | ||
| LogPrint("thin", "Received %s %s from peer %s (%d). Size %d bytes.\n", strCommand, inv.hash.ToString(), | ||
| pfrom->addrName.c_str(), pfrom->id, nSizeThinBlock); |
| CInv inv(MSG_BLOCK, thinBlock.header.GetHash()); | ||
| bool fAlreadyHave = false; | ||
|
|
||
| if (nHops > 0) |
There was a problem hiding this comment.
I there should be some comments regarding what hops is referring to...since we're wedding thinblocks and expedited...we should put some explanation as to why and what these hops are referring to..you could put it here or at the top of the function.
There was a problem hiding this comment.
Thanks for the review. Good idea, I'll add that with the formatting fix.
| // Is there a previous block or header to connect with? | ||
| { | ||
| LOCK(cs_main); | ||
| uint256 prevHash = thinBlock.header.hashPrevBlock; |
There was a problem hiding this comment.
I'm concerned about all the cs_main locks we're aquiring and releasing and aquiring again...i think we do this about 3 times here. While for thinblock processing it's not a big deal , for expedited it is important. We can easily a 1/10th of second waiting for a cs_main lock. I would look at consolidating them here so that we only take the lock once and hold it until we don't need it...so in checkng the POW (IsThinBlockValid), contextualcheckblockheader and also checking to see if the block connects...etc...all should be under one cs_main lock..rather than aquire/release/aquire/release...cs_main is recursive so you don't need worry about taking it twice, just do not release it until your done with it.
Other than that it all looks pretty good although i havn't tested yet.
There was a problem hiding this comment.
At present the PR is a minimal unification of the code which makes it easy to compare to before. Do you mind if locking is improved as a follow-up PR rather than combining it in this one?
A common function that unifies the two message handlers to have a single codepath for verification. The only differences lie in determining new-ness of the block. The prior expedited code path used to omit most of the validity checks.
|
I've fixed conflicts and did some log and formatting cleanup and reissued as PR #522. I moved to a new PR since the author is on vacation AFAIK. |
A common function that unifies the two message handlers to have a single codepath
for verification. The only differences lie in determining new-ness of the block.
The prior expedited code path used to omit most of the validity checks.
This is built on #482 and #502. Unfortunately they conflict (with trivial resolution as in this PR); it's probably cleanest to apply them in that order.