New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tracking of thin type blocks in flight. (pre-req for compact blocks) #1524

Merged
merged 7 commits into from Jan 4, 2019

Conversation

Projects
None yet
3 participants
@ptschip
Copy link
Collaborator

ptschip commented Dec 10, 2018

Move tracking of thin type block out of CNode and into a new "thintype" singleton
class. This removes a great deal of redundant code from thinblock.cpp and graphene.cpp
and makes the code much easier to maintain particularly in RequestBlock() in the
requestManager.cpp.

Consolidate the thinblock/grapheneblock timers and put them in the thintype
singleton class. This helps to further cleanup the logic in RequestBlock().

put a cap on inflight thin type

use multimap for thintype tracking which will eventually allow us to handle
multiple thin type blocks in flight for a single peer.

Use a system of atomic counters to determine how many of each type of peer
is currently connected.

@ptschip ptschip force-pushed the ptschip:dev_thintype branch 2 times, most recently from 93995c4 to 205fb94 Dec 10, 2018

@sickpig

This comment has been minimized.

Copy link
Collaborator

sickpig commented Dec 11, 2018

Great work Peter!

One minor nit: you are using "thin type block" to refer to blocks "encoded" by xthin or compact block or graphene. Am I correct?

If this is the case I found it a little bit confusing because we are already using "thin" blocks in the context of xthin.

In fact thin as block where we are sending all txids.

So I wonder if we could something else to label it for the former case (I have no suggestion in my mind, thou. sorry).

@ptschip

This comment has been minimized.

Copy link
Collaborator

ptschip commented Dec 11, 2018

@ptschip ptschip force-pushed the ptschip:dev_thintype branch from b78bd76 to 68b2d37 Dec 13, 2018

@sickpig sickpig added the P2P label Dec 13, 2018

@ptschip ptschip force-pushed the ptschip:dev_thintype branch 2 times, most recently from ce8454e to 10417d5 Dec 19, 2018

}
if (fCmpct)
{
nCompactBlockPeers++;

This comment has been minimized.

@gandrewstone

gandrewstone Jan 3, 2019

Collaborator

this API should have similar semantics as RemoveThinTypePeers. In other words, check the pfrom->fSupportsCompactBlocks variable rather than passing an override.

This comment has been minimized.

@ptschip

ptschip Jan 4, 2019

Collaborator

Done, but I had to turn it into two separate functions.

@@ -144,6 +145,10 @@ void InitializeNode(const CNode *pnode)

void FinalizeNode(NodeId nodeid)
{
// Decrement thin type peer counters
thinrelay.RemoveThinTypePeers(&(*connmgr->FindNodeFromId(nodeid)));

This comment has been minimized.

@gandrewstone

gandrewstone Jan 3, 2019

Collaborator

connmgr->FindNodeFromId(nodeid).get() is clearer than &* here.

This comment has been minimized.

@ptschip

ptschip Jan 4, 2019

Collaborator

done

std::atomic<int32_t> nCompactBlockPeers{0};

public:
void AddThinTypePeers(CNode *pfrom, bool fCmpct = false);

This comment has been minimized.

@gandrewstone

gandrewstone Jan 3, 2019

Collaborator

Nit:
These APIs are called in the context of the singleton class "thinrelay". So I do not think that you need to have the words "ThinType" in all these APIs, and having it makes them longer and wordier.

Give it a bit of thought but I'm OK either way.

This comment has been minimized.

@ptschip

ptschip Jan 4, 2019

Collaborator

would like to wait until I'm finished compact blocks since I'll just end up having to make a cleanup commit anyway.

// ensure that cs_inflight is locked for the duration of both IsThinTypeBlockInFlight()
// and AddThinTypeBlockInFlight, or we could end up downloading two of the same block
// caused by the multi-threading in message sending and processing.
thinrelay.AddThinTypeBlockInFlight(pfrom, inv2.hash, NetMsgType::GRAPHENEBLOCK);

This comment has been minimized.

@gandrewstone

gandrewstone Jan 3, 2019

Collaborator

This requirement for an atomic "if not then add" is very common. Let's make it one API (how about IfEmptyAddBlockInFlight) that returns true if it was added, false if it already existed. If you do this, we don't need the LOCK outside the API call, and so we don't need the LEAVE_CRITICAL and ENTER_CRITICAL. Those 2 APIs should be used as rarely as possible because they are not exception safe and I suspect don't play nicely with the debug mode deadlock detector.

This comment has been minimized.

@ptschip

ptschip Jan 4, 2019

Collaborator

yes, thanks, done

// ensure that cs_inflight is locked for the duration of both IsThinTypeBlockInFlight()
// and AddThinTypeBlockInFlight, or we could end up downloading two of the same block
// caused by the multi-threading in message sending and processing.
thinrelay.AddThinTypeBlockInFlight(pfrom, inv2.hash, NetMsgType::XTHINBLOCK);

This comment has been minimized.

@gandrewstone

gandrewstone Jan 3, 2019

Collaborator

same IfEmptyAdd... API needed here.

This comment has been minimized.

@ptschip

ptschip Jan 4, 2019

Collaborator

done

ptschip added some commits Dec 6, 2018

Refactor tracking of thin type blocks in flight.
Move tracking of thin type block out of CNode and into a new "thintype" singleton
class. This removes a great deal of redundant code from thinblock.cpp and graphene.cpp
and makes the code much easier to maintain particularly in RequestBlock() in the
requestManager.cpp.

Consolidate the thinblock/grapheneblock timers and put them in the thintype
singleton class. This helps to further cleanup the logic  in RequestBlock().

put a cap on inflight thin type

use multimap for thintype tracking which will eventually allow us to handle
multiple thin type blocks in flight for a single peer.

Use a system of atomic counters to determine how many of each type of peer
is currently connected.
Fix requestmanager_tests.cpp
Timer was returning false when it should return true.
@ptschip

This comment has been minimized.

Copy link
Collaborator

ptschip commented Jan 4, 2019

@ptschip ptschip force-pushed the ptschip:dev_thintype branch from 10417d5 to 2df2c75 Jan 4, 2019

@gandrewstone gandrewstone merged commit 5b52bb6 into BitcoinUnlimited:dev Jan 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment