320 rounds: expose block header from txtail #3935

algorandskiy · 2022-04-28T23:50:51Z

Make block header accessible from txtail with BlockHdrCached method
Some fixes to in-memory data representation
Added new consensus parameter to increase blocks lookback size
More comments and test

ledger/tracker.go

cce · 2022-05-05T16:23:55Z

ledger/txtail.go

@@ -207,17 +229,26 @@ func (t *txTail) committedUpTo(rnd basics.Round) (retRound, lookback basics.Roun
 		delete(t.lastValid, t.lowWaterMark)
 	}

-	return (rnd + 1).SubSaturate(maxlife), basics.Round(0)
+	deeperHistory := basics.Round(t.recent[rnd].proto.DeeperBlockHeaderHistory)


Should we consider removing proto from roundLeases now and switching to t.blockHeaderData[rnd]? Now we have two consensus versions/protos in the txtail row?

Okay, I tried this. Using t.blockHeaderData instead of t.recent requires taking a t.tailMu lock (t.blockHeaderData is changed in commitRound and Co and the race detector complains). t.recent is updated in newBlock and protected by l.trackerMu in ledger.
For some reason the old l.trackerMu lock does not lead to a horrible performance degradation as t.tailMu does. I need to look further, leaving as is now.

cce · 2022-05-05T16:24:18Z

ledger/txtail.go

 	bufIdx := 0
 	t.tailMu.RLock()
-	lastOffset := offset + maxTxnLife
+	lastOffset := offset + retianSize - 1


the loop below is supposed to fill retainSize elements starting from offset, this means it should make retainSize iterations and lastOffset is exclusive => size of the interval [offset, lastOffset) is lastOffset - offset = retainSize. Fixed

ledger/txtail.go

ledger/txtail_test.go

tolikzinovyev

Leaving some comments. Will continue review tomorrow.

config/consensus.go

ledger/accountdb.go

ledger/ledger.go

tolikzinovyev · 2022-05-05T23:04:33Z

ledger/ledger.go

+// and valid if txn.LastValid - txn.FirstValid <= MaxTxnLife
+// the deepest lookup happens when txn.LastValid == current => txn.LastValid == Lastest + 1
+// that gives Lastest + 1 - (MaxTxnLife + 1) = Lastest - MaxTxnLife as the first round to be accessible.
+func (l *Ledger) BlockHdrCached(rnd basics.Round) (bookkeeping.BlockHeader, error) {


Why not use this cache in BlockHdr() instead of making a new function?

the problem with differect code paths that call BlockHdr and eventually deadlock the tracker in initialization (namely ledger.(*accountUpdates).initializeFromDisk called from reloadLedger needs calls BlockHdr with trackers lock taken.

This requires deeper rework and getting rid of tr.mu or trackerMu mutexes.

Hm, it's strange that locks are needed at all during initialization.

ledger/ledger_test.go

codecov-commenter · 2022-05-06T03:17:12Z

Codecov Report

Merging #3935 (8cc23c1) into feature/320-rounds (1a3e310) will increase coverage by 0.06%.
The diff coverage is 83.51%.

@@                  Coverage Diff                   @@
##           feature/320-rounds    #3935      +/-   ##
======================================================
+ Coverage               50.13%   50.19%   +0.06%     
======================================================
  Files                     412      412              
  Lines                   70683    70701      +18     
======================================================
+ Hits                    35434    35488      +54     
+ Misses                  31384    31352      -32     
+ Partials                 3865     3861       -4

Impacted Files	Coverage Δ
ledger/trackerdb.go	`46.72% <ø> (ø)`
ledger/accountdb.go	`71.76% <40.00%> (-0.02%)`	⬇️
ledger/msgp_gen.go	`42.02% <66.66%> (-0.04%)`	⬇️
ledger/txtail.go	`91.61% <90.56%> (-1.52%)`	⬇️
config/consensus.go	`85.71% <100.00%> (+0.04%)`	⬆️
ledger/ledger.go	`65.04% <100.00%> (+0.32%)`	⬆️
ledger/tracker.go	`77.27% <100.00%> (+1.63%)`	⬆️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
catchup/service.go	`69.38% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a3e310...8cc23c1. Read the comment docs.

config/consensus.go

ledger/ledger.go

tolikzinovyev · 2022-05-06T18:41:14Z

ledger/ledger.go

+// and valid if txn.LastValid - txn.FirstValid <= MaxTxnLife
+// the deepest lookup happens when txn.LastValid == current => txn.LastValid == Lastest + 1
+// that gives Lastest + 1 - (MaxTxnLife + 1) = Lastest - MaxTxnLife as the first round to be accessible.
+func (l *Ledger) BlockHdrCached(rnd basics.Round) (bookkeeping.BlockHeader, error) {


Hm, it's strange that locks are needed at all during initialization.

ledger/ledger_test.go

tolikzinovyev · 2022-05-06T19:36:11Z

ledger/txtail.go

+	// The first entry matches that current tracker database round - MaxTxnLife (tail size is MaxTxnLife + DeeperBlockHeaderHistory)
+	// the second to tracker database round - (MaxTxnLife - 1), and so forth, and the last element is for the latest round.
+	// Deltas are in-memory not-committed-yet data.
+	blockHeaderData map[basics.Round]bookkeeping.BlockHeader


why is it a map and not an array like other fields?

tolikzinovyev · 2022-05-06T19:38:52Z

ledger/txtail.go

+	// preserve data for MaxTxnLife + DeeperBlockHeaderHistory
+	proto := config.Consensus[t.blockHeaderData[rnd].CurrentProtocol]
+	retainSize := proto.MaxTxnLife + proto.DeeperBlockHeaderHistory
+	newLowestRound := rnd.SubSaturate(basics.Round(retainSize)) + 1


Suggested change

newLowestRound := rnd.SubSaturate(basics.Round(retainSize)) + 1

newLowestRound := rnd.SubSaturate(basics.Round(retainSize) + 1)

no. say newBase = 10 and retain size is 3. Then the map should have 8, 9, 10 and newLowestRound = 8.
so newLowestRound = 10 - 3 + 1 = 8.

Sorry, I meant newLowestRound := rnd.SubSaturate(basics.Round(retainSize) - 1). The reason is that we don't want to remove round 0 when we shouldn't.

Alternatively:

newLowestRound := 0 if rnd + 1 > retainSize { newLowestRound = rnd + 1 - retainSize }

this makes sense, thank you for noticing.

tolikzinovyev · 2022-05-06T19:39:12Z

ledger/txtail.go

+	proto := config.Consensus[t.blockHeaderData[rnd].CurrentProtocol]
+	retainSize := proto.MaxTxnLife + proto.DeeperBlockHeaderHistory
+	newLowestRound := rnd.SubSaturate(basics.Round(retainSize)) + 1
+	if newLowestRound > 0 {


this check seems unnecessary

tolikzinovyev · 2022-05-06T19:46:40Z

ledger/txtail.go

+	proto := config.Consensus[t.blockHeaderData[rnd].CurrentProtocol]
+	retainSize := proto.MaxTxnLife + proto.DeeperBlockHeaderHistory
+	newLowestRound := rnd.SubSaturate(basics.Round(retainSize)) + 1
+	if newLowestRound > 0 {


Suggested change

if newLowestRound > 0 {

if newLowestRound > t.lowestBlockHeaderRound

otherwise increase in MaxTxnLife or DeeperBlockHeaderHistory can decrease t.lowestBlockHeaderRound

tolikzinovyev · 2022-05-06T19:50:31Z

ledger/txtail.go

+		newDeltaLength := len(t.roundTailSerializedDeltas)
+		firstTailIdx := len(t.roundTailHashes) - newDeltaLength - int(retainSize)
+		if firstTailIdx > 0 {
+			t.roundTailHashes = t.roundTailHashes[firstTailIdx:]


can't we just remove the first newLowestRound - t.lowestBlockHeaderRound elements similar to above?

tolikzinovyev · 2022-05-06T20:19:13Z

A thought: could we make DeeperBlockHeaderHistory a boolean? That way we don't need to worry about it being > 1. We can change it to integer in the future.

algorandskiy added Enhancement Team Carbon-11 labels Apr 28, 2022

algorandskiy requested a review from tolikzinovyev April 28, 2022 23:50

algorandskiy self-assigned this Apr 28, 2022

algorandskiy force-pushed the pavel/txtail-timestamp branch from cd15861 to e9605d3 Compare April 29, 2022 00:15

jannotti reviewed Apr 29, 2022

View reviewed changes

ledger/tracker.go Outdated Show resolved Hide resolved

Expose block timestamp and seed from txtail

db345fd

algorandskiy force-pushed the pavel/txtail-timestamp branch from e9605d3 to cf0528c Compare May 4, 2022 22:10

algorandskiy changed the title ~~320 rounds: expose block timestamp and seed from txtail~~ 320 rounds: expose block header from txtail May 4, 2022

Cache block headers in txtail

13570b7

algorandskiy force-pushed the pavel/txtail-timestamp branch from cf0528c to 13570b7 Compare May 5, 2022 01:07

algorandskiy requested a review from cce May 5, 2022 02:09

Introduce DeeperBlockHeaderHistory consensus parameter

bed45ba

algorandskiy force-pushed the pavel/txtail-timestamp branch from 299b79b to bed45ba Compare May 5, 2022 02:22

cce reviewed May 5, 2022

View reviewed changes

ledger/txtail.go Outdated Show resolved Hide resolved

cce reviewed May 5, 2022

View reviewed changes

ledger/txtail_test.go Outdated Show resolved Hide resolved

tolikzinovyev reviewed May 5, 2022

View reviewed changes

algorandskiy added 2 commits May 5, 2022 20:53

CR fixes

71e4d50

Restore params in t.recent

8cc23c1

tolikzinovyev reviewed May 6, 2022

View reviewed changes

CR 2 fixes

5263a1f

algorandskiy merged commit 62b7add into algorand:feature/320-rounds May 6, 2022

algorandskiy mentioned this pull request Jul 5, 2022

ledger: shorter deltas lookback implementation (320 rounds project) #4003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

320 rounds: expose block header from txtail #3935

320 rounds: expose block header from txtail #3935

algorandskiy commented Apr 28, 2022 •

edited

Loading

cce May 5, 2022

algorandskiy May 5, 2022

cce May 5, 2022

algorandskiy May 6, 2022

tolikzinovyev left a comment

tolikzinovyev May 5, 2022

algorandskiy May 6, 2022

algorandskiy May 6, 2022

tolikzinovyev May 6, 2022

codecov-commenter commented May 6, 2022 •

edited

Loading

tolikzinovyev May 6, 2022

tolikzinovyev May 6, 2022

tolikzinovyev May 6, 2022

algorandskiy May 6, 2022

tolikzinovyev May 6, 2022

algorandskiy May 6, 2022

tolikzinovyev May 6, 2022

tolikzinovyev May 6, 2022

algorandskiy May 6, 2022

tolikzinovyev May 6, 2022

tolikzinovyev commented May 6, 2022

	newLowestRound := rnd.SubSaturate(basics.Round(retainSize)) + 1
	newLowestRound := rnd.SubSaturate(basics.Round(retainSize) + 1)

	if newLowestRound > 0 {
	if newLowestRound > t.lowestBlockHeaderRound

320 rounds: expose block header from txtail #3935

320 rounds: expose block header from txtail #3935

Conversation

algorandskiy commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolikzinovyev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolikzinovyev commented May 6, 2022

algorandskiy commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented May 6, 2022 •

edited

Loading