Skip to content
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

Decouple peer-processing-logic from block-connection-logic #8865

Merged
merged 8 commits into from Oct 18, 2016

Conversation

@TheBlueMatt
Copy link
Contributor

commented Oct 2, 2016

This is part of a series of about 20 commits which split main.cpp into two - peer processing logic and blockchain/mempool/UTXO logic. Most of them arent much more intrusive than these, at least until there are large blocks of code moving.

This set focuses primarily on using CValidationinterface to handle peer-processing-logic updates when connecting/disconnecting blocks.

I haven't significantly tested this as my normal test machine is largely unavailable atm, but most of the changes here are pretty straight-forward.

@laanwj laanwj added the P2P label Oct 3, 2016

void InitPeerLogic(CConnman& connman) {
if (mapPeerLogics.count(&connman))
return;
mapPeerLogics.emplace(std::make_pair(&connman, std::unique_ptr<PeerLogicValidation>(new PeerLogicValidation(&connman))));

This comment has been minimized.

Copy link
@theuni

theuni Oct 3, 2016

Member

nit: don't make_pair with emplace, just use the components directly.

}
}
};
std::map<CConnman*, std::unique_ptr<PeerLogicValidation> > mapPeerLogics;

This comment has been minimized.

Copy link
@theuni

theuni Oct 3, 2016

Member

Using a ptr for the index feels kinda icky. Since a reference is passed and stored anyway, let's add an Id to CConnman instead. It'll be helpful in other places too.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 3, 2016

Author Contributor

Hmm, I feel like that is a layer violation? If CConnman is a low-level network socket/connection manager, it should have little knowledge of the fact that there is something hooked up above it to listen to validation callbacks and do things with them?

This comment has been minimized.

Copy link
@theuni

theuni Oct 3, 2016

Member

Sorry, I meant: add a GetID() to CConnman, which would return a const value set during construction. That would allow maps to be created which refer to specific connman instances without tying them to a pointer.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 3, 2016

Author Contributor

I think we're overengineering? Are we ever really gonna have more than one CConnman? Maybe something without even a map is simpler?

This comment has been minimized.

Copy link
@theuni

theuni Oct 3, 2016

Member

I'm hoping to be able to hook up 2 CConnmans to eachother in the future for testing against eachother, yes. I've gone to significant efforts to remove the "stuff a static global somewhere" practice from much of the net code, please don't start reintroducing.

A pointer is fine here if you'd prefer to keep it simple, we can adapt later if needed.

}
}
if (it != mapBlockSource.end())
mapBlockSource.erase(it);

This comment has been minimized.

Copy link
@theuni

theuni Oct 4, 2016

Member

Hmm, were these not erased before in the invalid case?

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 4, 2016

Concept ACK and quick code-review ACK other than the nits. I'd like to spend some time testing, though.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

No? I think after this patch set the only place mapBlockSource is erased from is this line.

On October 3, 2016 8:26:53 PM EDT, Cory Fields notifications@github.com wrote:

theuni commented on this pull request.

  •    const uint256 hash(block.GetHash());
    
  •    std::map<uint256, NodeId>::iterator it =
    
    mapBlockSource.find(hash);
    +
  •    int nDoS = 0;
    
  •    if (state.IsInvalid(nDoS)) {
    
  •        if (it != mapBlockSource.end() && State(it->second)) {
    
  •            assert (state.GetRejectCode() < REJECT_INTERNAL); //
    
    Blocks are never rejected with internal reject codes
  •            CBlockReject reject = {(unsigned
    
    char)state.GetRejectCode(), state.GetRejectReason().substr(0,
    MAX_REJECT_MESSAGE_LENGTH), hash};
  •            State(it->second)->rejects.push_back(reject);
    
  •            if (nDoS > 0)
    
  •                Misbehaving(it->second, nDoS);
    
  •        }
    
  •    }
    
  •    if (it != mapBlockSource.end())
    
  •        mapBlockSource.erase(it);
    

Hmm, were these not erased before in the invalid case?

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#8865 (review)

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 4, 2016

I was just pointing out that this looks to be a memleak bugfix :)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

I dont believe it is actually any (much?) different than it used to be. Maybe it will be deleted more often on reorgs now, I didnt investigate fully. In any case, its pretty hard to DoS since you only add to the map after AcceptBlock succeeds (ie the block is syntactically valid/connects/has valid PoW)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Switched g_connman to be a shared_ptr instead of a unique_ptr, which I think should neatly address all the outstanding nits.

TheBlueMatt added 2 commits Sep 30, 2016
Make validationinterface.UpdatedBlockTip more verbose
In anticipation of making all the callbacks out of block processing
flow through it. Note that vHashes will always have something in it
since pindexFork != pindexNewTip.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:net_processing_1 branch from 7366465 Oct 4, 2016

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Backed out the shared_ptr change, I misunderstood @theuni's comments, just fixed the make_pair nit and left everything else as-is: it should be easier to clean up things like class design once everything is split out.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:net_processing_1 branch to a9aec5c Oct 4, 2016

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Restructured the code after discussion on IRC with @theuni...now the CValidationInterface subclass is defined in main.h (requires an extra include, but we can drop that when we move all the stuff to another file) and init.cpp itself does the object initialization and keeps the CValidationInterface around.

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

utACK a9aec5c

@laanwj laanwj merged commit a9aec5c into bitcoin:master Oct 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Oct 18, 2016
Merge #8865: Decouple peer-processing-logic from block-connection-logic
a9aec5c Use BlockChecked signal to send reject messages from mapBlockSource (Matt Corallo)
7565e03 Remove SyncWithWallets wrapper function (Matt Corallo)
12ee1fe Always call UpdatedBlockTip, even if blocks were only disconnected (Matt Corallo)
f5efa28 Remove CConnman parameter from ProcessNewBlock/ActivateBestChain (Matt Corallo)
fef1010 Use CValidationInterface from chain logic to notify peer logic (Matt Corallo)
aefcb7b Move net-processing logic definitions together in main.h (Matt Corallo)
0278fb5 Remove duplicate nBlocksEstimate cmp (we already checked IsIBD()) (Matt Corallo)
87e7d72 Make validationinterface.UpdatedBlockTip more verbose (Matt Corallo)
connman->ForEachNode([nNewHeight, nBlockEstimate, &vHashes](CNode* pnode) {
if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) {
connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {

This comment has been minimized.

Copy link
@theuni

theuni Oct 18, 2016

Member

If pnode->nStartingHeight is -1, we just haven't completed the version handshake, no? Shouldn't we always refuse to relay in that case?

Or am I not seeing some other way this can happen?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 18, 2016

Author Contributor

Indeed, probably, though that would be a behavior change whereas this PR shouldnt have any :p.

@@ -50,6 +48,8 @@ class CValidationInterface {
struct CMainSignals {
/** Notifies listeners of updated block chain tip */
boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip;
/** A posInBlock value for SyncTransaction which indicates the transaction was conflicted, disconnected, or not in a block */
static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1;

This comment has been minimized.

Copy link
@theuni

theuni Oct 18, 2016

Member

This is fine for now, but I think we ultimately want these split up.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 18, 2016

Author Contributor

Yea, I think medium-term we need to think hard about what goes in what signaling interface (we have two already that are highly duplicative, I have some thoughts but we should discuss)

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

post-merge utACK.

}
// Relay inventory, but don't relay old inventory during initial block download.
connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {

This comment has been minimized.

Copy link
@rebroad

rebroad Oct 19, 2016

Contributor

The logic of this line is confusing now... If pnode->nStartingHeight = -1 then don't do (if nNewHeight > -1) but instead do (if nNewHeight > 0)?! It hardly makes any difference, it will return true, therefore simply do if (nNewHeight > pnode->nStartingHeight - 2000) without any need for the ? operator.

Anyway, why make it always true and not make use of the nBlocksEstimate as was previously done?

This comment has been minimized.

Copy link
@rebroad

rebroad Oct 19, 2016

Contributor

Fixed in #8958

This comment has been minimized.

Copy link
@sipa

sipa Oct 30, 2016

Member

See the earlier commit: "Remove duplicate nBlocksEstimate cmp (we already checked IsIBD())".

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

Posthummus utACK.

gmaxwell added a commit to gmaxwell/bitcoin that referenced this pull request Dec 6, 2016
zkbot added a commit to zcash/zcash that referenced this pull request Jul 16, 2018
Auto merge of #3263 - str4d:ibd-upstream-changes, r=<try>
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388
zkbot added a commit to zcash/zcash that referenced this pull request Jul 17, 2018
Auto merge of #3263 - str4d:ibd-upstream-changes, r=bitcartel
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388
zkbot added a commit to zcash/zcash that referenced this pull request Jul 17, 2018
Auto merge of #3263 - str4d:ibd-upstream-changes, r=bitcartel
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.