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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Merged by Bors] - in memory index with atx data for consensus layer #5013
[Merged by Bors] - in memory index with atx data for consensus layer #5013
Changes from 23 commits
b3fbc3e
d5e1e10
e4009bd
dc74a14
f3b4fdb
bcbb769
22dd804
bfda8d9
2ed1e21
fd8b5aa
94d20be
d976e04
b9f59b2
14338fe
5cc11d6
ee6a98b
7f47c0f
2b0410f
5a70266
4c790c3
2a9830c
d6f896b
31f226f
c0f7a20
e887d07
b510321
fcf9e2e
ba31735
dbfee1c
5eefe50
7782971
edd6406
8990e36
e6d1ca4
de00596
4696054
184b9e3
36a02f8
1d3d3ea
6a65672
7986ca5
c62029e
15644cf
9959c38
85e48c8
fbf6667
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
i think nonce and malicious is a property of identity, not atx.
if an identity is marked malicious in epoch 3, it should still be malicious in epoch 6. but with current design if nodes didn't restart btwn these time it will be forgotten.
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.
it doesn't matter whose property logically is it. it is about how data is stored and indexed.
identity is relevant in consensus only if it has atx in this epoch.
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.
this is theoretically true, but in practice we already have this in hare/tortoise where we use nodeid->malicious mapping to avoid checking nodeid->"atx in this epoch" lookup.
and cachedb's IsMalicious() is also by nodeid. how do you plan to replace that?
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.
hare GetByNode, tortoise Get
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.
specifically hare will use only fastpath that relies on consensus data, it won't even need to have references to db for that.
tortoise (proposal/ballots handler) will query using Get(atx)
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.
i don't think we need this. the identity should already be set malicious when Cache.SetMalicious() is called.
and at that point it shouldn't matter which ATX is returned for a malicious identity.
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.
we need to store them to support NodeHasAtx. otherwise ballots won't be accepted. and for consistency it is better to keep them ordered
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 plan to only store the first equivocating atx/ballot and only sync those when they are referenced.
i see NodeHasAtx is used to check proposal eligibility. do you think this step is needed if the ballot is malicious?
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.
why does it matter?
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.
don't understand. if a reference is stored in the map, why not update it directly?
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.
it will be a race with returned values
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.
sorry still not clear. do you mean there will be a race if you do
ecache.atxs[atx].Malicious = true
directly?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.
pointer is returned in response
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.
map stores a pointer, and it returns it to avoid copy on every read. during update that pointer is not protected by mutex, so if we update that memory directly race will be noticed and reported. therefore we copy it, update, and reinsert into map that is protected by mutex
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.
thanks for the explanation. i've made mistakes like this a few times.