Algod: Add a new caching mechanism for the ledger - latest 512 block headers #3765

Aharonee · 2022-03-14T12:14:56Z

Generating the State Proof message periodically requires building a merkle tree from the previous 256 (and in the future perhaps 64) block headers.

This cache will keep recent block headers in memory, improving the time required for fetching them by an order of magnitude.

… headers.

id-ms

overall looks pretty good!
just a few minor changes

ledger/blockHeadersCache.go

ledger/blockHeaderCache.go

ledger/blockHeaderCache_test.go

algonautshant

Looks great.
Some optimizations and questions.

Also, the PR is missing explanation: please add a few words about the purpose of this.

ledger/blockHeaderCache.go

Co-authored-by: Shant Karakashian <[email protected]>

ledger/blockHeaderCache.go

tsachiherman · 2022-03-17T13:36:50Z

ledger/blockHeaderCache.go

+	"github.com/algorand/go-deadlock"
+)
+
+const cacheSize = 512


this isn't a great name for a global variable. But more important, I don't think that it should be a const anyway.
I believe that what you intended to have here is the CompactCertRounds * 2, and if so, we should make it depending on that value. Given that you need a constant value here, you could create a MaxCompactCertRounds in the config package and use it here.

currently, we have a test that breaks if this number is not optimal for the current CompactCertRounds.

I'm not sure I like this value to be implicitly changed if the CompactCertRounds changes.
From my perspective, we should have a test that breaks and someone explicitly changes it if needed.

That's fine - in that case, I have two requests -;)

change the variable name to so that it would not potentially conflict. i.e. blockHeadersSlidingWindowCacheSize or similar.

add a comment on top of this constant, describing why this value was selected, and specifying the name of the unit test that validate the correctness of this value.

ledger/blockHeaderCache.go

tsachiherman · 2022-03-17T13:39:16Z

ledger/blockHeaderCache.go

+	generating the stateproof message will be sequential in memory.
+	Might improve performance in terms of CPU caching.
+	*/
+	idx := basics.SubSaturate(uint64(round), 1) % cacheSize


I think that the basics.SubSaturate here ( and below ) isn't required. I agree that sequential ordering is good, but it doesn't really matter if you shift the array by one or not.

idx := (round - 1 ) % latestCacheSize should be the same in my opinion.

round -1 might overflow and I don't really like it. that's why I suggested using basics.SubSaturate. @tsachiherman do you have a suggestion of how to keep the blockheaders sequentially in memory and not use the -1?

yes, just use idx := round % cacheSize. It would still keep the sequential ordering, but would not "rotate" the array. It doesn't really give you any advantage to shift it as long as you're doing it symmetrically.

In order to construct stateproof message for interval 512 (for example) we need to gather all the headers for rounds : [ 257, 258 ... 512]. If we use idx := round % cacheSize we end up with the last round (512) placed in index 0 in the cache. I would like the order of the cache to reflect the order of fetching the message

Ah - I see. I think that when you'll get to the point of actually getting these headers, you'll see that it won't help you.
i.e. you can't return a slice of [257:] since the backing array might get changed. Instead, you'll need to create a copy of the backing array... and if you're creating a copy of the backing array, the index plays a smaller part.

Also, you could calculate the index in a more "branchless" way:

idx := (round + cacheSize - 1) % cacheSize

either way, I'd encourage you to implement all the methods you would need on this cache as well as to document the rationale for non-trivial indexing choices.

ledger/blockHeaderCache.go

codecov-commenter · 2022-03-20T11:31:11Z

Codecov Report

Merging #3765 (fcd14b0) into feature/stateproofs (f9a8d27) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##           feature/stateproofs    #3765   +/-   ##
====================================================
  Coverage                49.61%   49.61%           
====================================================
  Files                      391      392    +1     
  Lines                    68529    68553   +24     
====================================================
+ Hits                     33998    34011   +13     
- Misses                   30787    30804   +17     
+ Partials                  3744     3738    -6

Impacted Files	Coverage Δ
ledger/blockHeaderCache.go	`100.00% <100.00%> (ø)`
ledger/ledger.go	`59.53% <100.00%> (-0.14%)`	⬇️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
node/node.go	`23.48% <0.00%> (-1.87%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-0.87%)`	⬇️
network/wsNetwork.go	`62.79% <0.00%> (ø)`
catchup/service.go	`70.12% <0.00%> (+0.74%)`	⬆️
util/metrics/counter.go	`80.95% <0.00%> (+2.38%)`	⬆️
util/metrics/gauge.go	`70.66% <0.00%> (+2.66%)`	⬆️
... and 1 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 f9a8d27...fcd14b0. Read the comment docs.

jannotti · 2022-10-11T09:12:29Z

There are couple other places we need (or will need) access to old headers. AVM code should be allowed to access the LatestTimestamp from previous headers, and an upcoming opcode, `block_seed` should be able to as well. They will need to look back at least MaxTxLiftetime (1000). (Maybe longer to support catchup.). It would be nice if all of these use cases use the same mechanism.

…

On Mon, Mar 14, 2022 at 8:15 AM Or Aharonee ***@***.***> wrote: ------------------------------ You can view, comment on, or merge this pull request online at: #3765 Commit Summary - 530fbde <530fbde> Implemented a new caching mechanism for the ledger - latest 512 block headers. - 7fc60af <7fc60af> Fix small bug and add init for cache - de60364 <de60364> Add unit tests File Changes (3 files <https://github.com/algorand/go-algorand/pull/3765/files>) - *A* ledger/blockHeadersCache.go <https://github.com/algorand/go-algorand/pull/3765/files#diff-529321c786df1373ae8db05dc01aa97468f8600939ddc3abb1deea06c510ea03> (87) - *A* ledger/blockHeadersCache_test.go <https://github.com/algorand/go-algorand/pull/3765/files#diff-606048023c00fdc6ef18f3be13d0ee3d449dd5d4c58ef8f2dd8f23bee1fe1137> (73) - *M* ledger/ledger.go <https://github.com/algorand/go-algorand/pull/3765/files#diff-362fe6e8cc259e0660b63453dc0c87f2e19be9f4a31b056be34d692eb138cba0> (7) Patch Links: - https://github.com/algorand/go-algorand/pull/3765.patch - https://github.com/algorand/go-algorand/pull/3765.diff — Reply to this email directly, view it on GitHub <#3765>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADL7TZQE4PCKY5UYXKXAE3U74UUXANCNFSM5QVKZXLA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

Aharonee added 3 commits March 13, 2022 16:34

Implemented a new caching mechanism for the ledger - latest 512 block…

530fbde

… headers.

Fix small bug and add init for cache

7fc60af

Add unit tests

de60364

Aharonee requested review from id-ms and tsachiherman March 14, 2022 12:14

small fix to unit test

eedacab

Aharonee changed the title ~~Add a new caching mechanism for the ledger - latest 512 block headers~~ Algod: Add a new caching mechanism for the ledger - latest 512 block headers Mar 14, 2022

Aharonee added the Enhancement label Mar 14, 2022

id-ms assigned Aharonee Mar 15, 2022

id-ms added the Team Sphinx label Mar 15, 2022

id-ms force-pushed the feature/stateproofs branch from b59ab51 to 41277ab Compare March 15, 2022 11:04

added some benchmarks and unit test

f78e5da

id-ms requested a review from algonautshant March 15, 2022 17:14

Aharonee added 2 commits March 16, 2022 11:10

rename

a1e9479

change Put signature

eae7e56

Aharonee force-pushed the stateproof-add_block_headers_cache branch from 9345ada to eae7e56 Compare March 16, 2022 09:27

Aharonee added 3 commits March 16, 2022 11:34

refactor

3a6cfc5

dog

24c5345

.

c9e4114

id-ms reviewed Mar 16, 2022

View reviewed changes

ledger/blockHeadersCache.go Outdated Show resolved Hide resolved

ledger/blockHeaderCache.go Outdated Show resolved Hide resolved

ledger/blockHeaderCache_test.go Outdated Show resolved Hide resolved

ledger/blockHeaderCache_test.go Show resolved Hide resolved

Aharonee added 2 commits March 16, 2022 15:55

refactor, add some comments and extend unit tests

ce4e8f0

fix test

75f4abd

algonautshant reviewed Mar 17, 2022

View reviewed changes

Aharonee and others added 3 commits March 17, 2022 11:13

Update ledger/blockHeaderCache.go

d64a375

Co-authored-by: Shant Karakashian <[email protected]>

Update ledger/blockHeaderCache.go

0d6832b

Co-authored-by: Shant Karakashian <[email protected]>

more refactoring

a2ac822

tsachiherman reviewed Mar 17, 2022

View reviewed changes

ledger/blockHeaderCache.go Outdated Show resolved Hide resolved

tsachiherman reviewed Mar 17, 2022

View reviewed changes

ledger/blockHeaderCache.go Show resolved Hide resolved

tsachiherman reviewed Mar 17, 2022

View reviewed changes

algonautshant reviewed Mar 17, 2022

View reviewed changes

ledger/blockHeaderCache.go Outdated Show resolved Hide resolved

algonautshant approved these changes Mar 17, 2022

View reviewed changes

last touches and renaming

1dc3caf

Aharonee force-pushed the stateproof-add_block_headers_cache branch from fcd14b0 to 1dc3caf Compare March 20, 2022 14:41

id-ms merged commit 3dd4c6b into algorand:feature/stateproofs Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algod: Add a new caching mechanism for the ledger - latest 512 block headers #3765

Algod: Add a new caching mechanism for the ledger - latest 512 block headers #3765

Aharonee commented Mar 14, 2022 •

edited

Loading

id-ms left a comment

algonautshant left a comment

tsachiherman Mar 17, 2022

id-ms Mar 17, 2022

tsachiherman Mar 17, 2022

tsachiherman Mar 17, 2022 •

edited

Loading

algonautshant Mar 17, 2022

id-ms Mar 17, 2022 •

edited

Loading

tsachiherman Mar 17, 2022

id-ms Mar 17, 2022

tsachiherman Mar 17, 2022

codecov-commenter commented Mar 20, 2022 •

edited

Loading

jannotti commented Oct 11, 2022 via email

Algod: Add a new caching mechanism for the ledger - latest 512 block headers #3765

Algod: Add a new caching mechanism for the ledger - latest 512 block headers #3765

Conversation

Aharonee commented Mar 14, 2022 • edited Loading

id-ms left a comment

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

tsachiherman Mar 17, 2022

Choose a reason for hiding this comment

id-ms Mar 17, 2022

Choose a reason for hiding this comment

tsachiherman Mar 17, 2022

Choose a reason for hiding this comment

tsachiherman Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

algonautshant Mar 17, 2022

Choose a reason for hiding this comment

id-ms Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

tsachiherman Mar 17, 2022

Choose a reason for hiding this comment

id-ms Mar 17, 2022

Choose a reason for hiding this comment

tsachiherman Mar 17, 2022

Choose a reason for hiding this comment

codecov-commenter commented Mar 20, 2022 • edited Loading

Codecov Report

jannotti commented Oct 11, 2022 via email

Aharonee commented Mar 14, 2022 •

edited

Loading

tsachiherman Mar 17, 2022 •

edited

Loading

id-ms Mar 17, 2022 •

edited

Loading

codecov-commenter commented Mar 20, 2022 •

edited

Loading