This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Approval voting overlay db #3366
Approval voting overlay db #3366
Changes from 6 commits
55afad7
a7a612a
5bf68b0
844e56b
3e9e5f3
12899aa
7b9ff0d
46df410
103b873
7ae0dfa
ef68ae1
ffabf96
6b0f048
4e8dde8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer this to be a getter
fn config(&self) -> &Config
to protect mutability of the config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok(())
here is redundant if the?;
is omitted from the above line, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is the current impl does an implicit type conversion, if the returned Error types are the same, it indeed can be omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this implicit encoding set with
Option
s, could we not storeBackendWriteOp
directly? It costs a few extra bytes for the hash being present in addition, but from a clarity pov this would be advantageous.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually a great suggestion... I would suggest we merge this PR and then follow-up in an additional PR, but more than happy to explore the right approach in this PR.
Paging @rphmeier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically, instead of a
HashMap<K, Option<V>>
, you are suggesting we useHashMap<K, BackendWriteOp>
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the idea there be to have a single map
K -> BackendWriteOp
? Otherwise I think it could get less clear, not more, because the type system would then express the possibility to have aBackendWriteOp::WriteCandidateEntry
in theblock_entries
map, for example.Reusing
Option
for this is a little semantically wrong, but maybe a new enum is a better solution: