For inclusion in TRB, a feature was proposed, that I intend to comment on in this article.
Sync speed hypothesis
Based on looking at the code, my hypothesis of the expected time the cement sync would take, is: verification time + peer-average transfer time.
By peer-average here, I mean the average determined by all of the uploading peers' upload speed, weighted by the amount of participation. A detailed explanation follows:
Sync mechanics
At startup, vNodes is empty, until populated with peers a connection was established as the first. More peers may be added later.
On sync start, or on a peer lock-in break, cement sync continues with "one of the peers in vNodes".
Which one of the peers in vNodes, it depends on:
1) Whether cs_vSend is locked for a peer or not (outgoing data being handled at the same time (here and here) , or not), and either allowing SendMessages() processing at that moment, or not.
2) Message handler thread loop timing (spins through vNodes repeatedly, 100 ms delay in-between).
3) The order the peers ended up into vNodes in.
The sync "tends to" lock-in to a single peer, because when a block is accepted from peer, next block's AskFor() schedule starts off with the same peer. The first AskFor() of an item always "executes immediately", and further requests for the same item are only scheduled for later, in 2-minute intervals.
The lock-in may still break, though, if the locked-in peer's cs_vSend happened to be locked at a specific moment. The lock-in can also break, if peer fails to send the block before another, later-scheduled peer does.
When the lock-in breaks, the sync continues with the "one of the peers in vNodes", similarly as it started off with.
Interpretation
The effects of the occasional lock-in breaks by the cs_vSend being locked at specific moment, and peer failing to deliver the block in time, are assumed to average as random over time. This in turn would effectively randomize the block-sending peers, and hence, every peer we're connected to, get to affect our download speed equally.
How often will the peer lock-in break occur, though, I have no estimate of.
Slightly spammy AskFor()-scheduling
The mapAskFor of each peer may extend up to 10 * peer count / sec, for 2 minutes, until the currently expected block is received and accepted. Extending happens only during download-time, and not while verifying a block.
In a rough worst-case scenario, this could mean 10 * 125 * 2*60 * (8+4+32) ~ 6 MB of excess mapAskFor items.
In the light of 1) what other spit-spaghetti TRB consists of, and 2) not immediately knowing any obviously better approach, this kind of issue may not call for immediate action, but perhaps worth knowing still.
Cement loading
The loadcement RPC-command's help text should mention the cement to be loaded must be in ascending order, and continuous, even if loaded in multiple batches.
About the validation in Cement::load():
1) It should say it explicitly, when the problem is a gap. Accidental gap, when feeding multiple cement files could get hard for the operator to notice.
2) Why not just stop the cement loading, immediately on any occurred problem?
2a) A gap in cement will cause possibly a large part of the cement to get dumped into console/debug.log, for nothing.
2b) Any kind of "natural rot" error is effectively also a gap, causing also an unwanted console/log dump.
With the above changes, two further points are worth noting:
1) Cement::load() summary can now state a (better informing) height range, instead of a simple, loaded hash count.
2) The gap check in Cement::verify() is now possibly redundant.
Other notes
When the classical mining is enabled, ProcessBlock() is used to self-validate new blocks. Cement-validating new, unpublished blocks doesn't make sense. The cement mode, and mining mode should be mutually exclusive.
I refrained from commenting the thread lock earlier, but now feel ready for it: The thread lock is necessary for the cement functionality, as it currently stands. RPC-commands are processed by one thread, and incoming messages by another. Without the lock, one race condition example would be:
The operator could be loading more cement from a file, while an incoming block was being checked against the cement. Also, the miner thread may trigger a cement verification, even though it shouldn't.
Somewhat inconsistent naming is used, and possibly could be improved: "permitCement", "cementPermitted", "mayLoadCement", and "-cement"; all meaning the exact same thing: Whether the cement-mode is enabled or not.