Introduce caching of PowerTable CIDs #784
Conversation
Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
feat/ptcid-cache
branch
3 times, most recently
from
8417b74 to
6140b92
Compare
Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu
force-pushed
the
feat/ptcid-cache
branch
from
6140b92 to
302fbdb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #784 +/- ##
==========================================
- Coverage 69.79% 69.42% -0.37%
==========================================
Files 76 77 +1
Lines 7870 7896 +26
==========================================
- Hits 5493 5482 -11
- Misses 1931 1961 +30
- Partials 446 453 +7
|
| cache, err := lru.New[string, cid.Cid](256) // keep a bit more than 2x max ECChain size | ||
| if err != nil { | ||
| // panic as it only depends on the size | ||
| panic(fmt.Errorf("could not create cache: %w", err)) |
There was a problem hiding this comment.
I would return an error based on the principle that panics should be used rarely. I understand there are pre-existing practices in this repo on use of panic. So not a blocker for me.
There was a problem hiding this comment.
It is a pet peeve of mine not to introduce errors, especially in constructors, when they cannot happen.
| } | ||
|
|
||
| func (h *gpbftInputs) getPowerTableCIDForTipset(ctx context.Context, tsk gpbft.TipSetKey) (cid.Cid, error) { | ||
| sTSK := string(tsk) |
There was a problem hiding this comment.
How big do tipset keys get? do we want to hash these?
There was a problem hiding this comment.
on expectation ~5x the hash size
| return ptCid, nil | ||
| } | ||
|
|
||
| func (h *gpbftInputs) collectChain(ctx context.Context, base ec.TipSet, head ec.TipSet) ([]ec.TipSet, error) { |
There was a problem hiding this comment.
This function would collect the entire chain that could be finalised irrespective of max chain length of 100. Right?
Specifically, during bootstrap this function would, at best, collect a chain of 900, then a chain of 800, 700, and so on even though each instance would only use at most 100 tipsets of the returned chain as proposal.
Is that the most efficient we can be here? How fast/expensive is GetParent?
There was a problem hiding this comment.
It isn't that expensive in comparison to getting the power table, it could be more efficient but it is tricky given null rounds.
Co-authored-by: Masih H. Derkani <[email protected]>
First commit is a refactor, second one is the change.