commit to Merkle tree of online participants in block header #1447
Conversation
Instead, add the normalized balance as a separate column to the existing accountbase table, and construct an index on that table.
Resolve conflict in ledger/accountdb_test.go
the cert verification logic only checks the last reveal, since the reveals get placed in a map, so duplicate reveals cause some reveals to not be checked at all.
ledger/eval.go
Outdated
| @@ -762,6 +763,18 @@ func (eval *BlockEvaluator) endOfBlock() error { | |||
| } else { | |||
| eval.block.TxnCounter = 0 | |||
| } | |||
|
|
|||
| if eval.proto.CompactCertRounds > 0 && eval.block.Round()%basics.Round(eval.proto.CompactCertRounds) == 0 { | |||
There was a problem hiding this comment.
consider changing the CompactCertVoters(loopback) so that it have a similar signature to something like:
err := CompactCertVoters(basics.Round, *CompactCertVoters, *CompactCertVotersTotal)
so that you could collapse the logic you’ve added to finalValidation and endOfBlock into a single location.
There was a problem hiding this comment.
The reason the Ledger.CompactCertVoters() function returns the entire VotersForRound is that I need other parts from that VotersForRound in the rest of the code. Here's the complete compactcert implementation, if it's helpful, and the other place that I need to use Ledger.CompactCertVoters(): https://github.com/zeldovich/go-algorand/blob/compactcert/compactcert/builder.go#L48
But I agree it'd be good to factor out this common logic. Pushed that change.
| // construct this Merkle tree, so as to avoid placing the | ||
| // construction of this Merkle tree (and obtaining the requisite | ||
| // accounts and balances) in the critical path. | ||
| CompactCertVotersLookback uint64 |
There was a problem hiding this comment.
Do you have any suggestion for good numbers for CompactCertVotersLookback and CompactCertRounds ?
There was a problem hiding this comment.
CompactCertVotersLookback is relatively easy: something like 16 should be fine.
CompactCertRounds depends a lot on where we imagine using these compact certs, because it translates into how often you have to check compact certs (per unit time) to keep up with the state of the chain. I've been thinking of this being somewhere around 128-1024. Probably not smaller. Perhaps a bit longer depending on the use case (e.g., I could imagine this being 8K or 16K, so a compact cert once a day..)
| var totalWeight basics.MicroAlgos | ||
|
|
||
| for i, acct := range top { | ||
| var ot basics.OverflowTracker |
There was a problem hiding this comment.
nit: move the basics.OverflowTracker outside the for loop.
There was a problem hiding this comment.
You're right that we could move it out (since, on each iteration of the loop, either ot remains non-overflowed, or the loop returns an error). But I think that would make the code harder to read, because ot is used independently for each iteration of the loop, and it's not used after the loop.
There was a problem hiding this comment.
I think that overall, it looks great. One thing to add :
the votersTracker would need a "close()" function the waits until all the internal goroutines are done, and reset it's state so that when loadFromDisk is called, it would not retain any previous state.
You can do that by replacing the votesTracker in the acctupdates.go, or implement a cleanup in the votesTracker itself.
|
Ah, good point about resetting the votersTracker on I think this is more-or-less good to go, then. I'll rebase against master and remove the draft flag when #1359 and #1361 get approved and merged. |
|
|
||
| // Wait for the Merkle tree to be constructed. | ||
| tr.mu.Lock() | ||
| for tr.Tree == nil { |
There was a problem hiding this comment.
this could lead to a hang in case we had error during the VotersForRound::loadTree. maybe store the error generated during the VotersForRound::loadTree inside the VotersForRound, and make sure to broadcast when changing the error as well ?
There was a problem hiding this comment.
That seems like a good plan. I think originally I had no failure paths in the background loadTree() function, but indeed now there's possible errors that can cause this to hang forever. Pushed a revised version.
zeldovich
changed the base branch from
nickolai/compactcert-top-crypto
to
master
| func PendingRewards(ot *OverflowTracker, proto config.ConsensusParams, microAlgos MicroAlgos, rewardsBase uint64, rewardsLevel uint64) MicroAlgos { | ||
| rewardsUnits := microAlgos.RewardUnits(proto) | ||
| rewardsDelta := ot.Sub(rewardsLevel, rewardsBase) | ||
| return MicroAlgos{Raw: ot.Mul(rewardsUnits, rewardsDelta)} | ||
| } |
There was a problem hiding this comment.
seems like this function needs to called in WithUpdatedRewards right below
There was a problem hiding this comment.
Indeed it's basically the same code. The reason I didn't merge them is that I liked the detailed panic message that describes the internal computation steps that overflowed (in the off chance that we do trigger overflow), and the function below does one more overflow-checked addition that could overflow and cause that message to be printed (so it's not just a matter of moving the panic-on-error into this new PendingRewards())..
This code is rather deeply embedded in our protocol now, so it seemed not so bad to have a copy of this code, since it's not so likely to change.
| // The votersTracker maintains the Merkle tree for the most recent | ||
| // commitments to online accounts for compact certificates. | ||
| // | ||
| // We maintain multiple Merkle trees: we might commit to a new Merkle tree in |
There was a problem hiding this comment.
Can you please give an example on how there are multiple Merkle trees?
There was a problem hiding this comment.
What the comment refers to is that there might be multiple elements in the round map, and each one has a Merkle tree.
| // are formed for blocks that are a multiple of CompactCertRounds, using | ||
| // the Merkle commitment to online accounts from the previous such block. | ||
| // Thus, we maintain X in the round map until we form a compact certificate | ||
| // for round X+CompactCertVotersLookback+CompactCertRounds. |
There was a problem hiding this comment.
if CompactCertVotersLookback is much smaller than CompactCertRounds, can this be X+CompactCertRounds?
There was a problem hiding this comment.
It is indeed the case that CompactCertVotersLookback is on the order of 16, and CompactCertRounds is probably something like 128 or 1024.
However, I believe the comment is correct: the two intervals have to be added together. We need the compact cert to certify the next block that is a multiple of CompactCertRounds, because that block will contain the commitment to the next group of voters (which will enable verifying the next compact cert, etc). And the CompactCertVotersLookback addition is needed to give the ledger time to compute the Merkle tree, in case there are many voters.
…d#1447) commit to Merkle tree of online participants in block header
This PR commits to a Merkle tree of online participants in the block header, for use in validating compact certificates.
The main tricky aspect of this PR has to do with the fact that it takes a while to compute this Merkle tree, so the computation needs to happen over the course of several blocks, before the ledger can expect the Merkle root to be available.