Skip to content
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

Handle acting on tallied pieces of data that take longer than one round to confirm #768

Merged
merged 58 commits into from
Dec 8, 2022

Conversation

james-chf
Copy link
Contributor

@james-chf james-chf commented Nov 10, 2022

Closes #197

This PR makes it so that votes are applied for things (e.g. Ethereum events) which already exist in storage but that are not yet seen = true. There are also a couple of small changes to the FractionalVotingPower type.

@james-chf james-chf force-pushed the james/ethbridge/multiple-votes branch from cff7a1f to 535e4be Compare November 10, 2022 12:37
@james-chf james-chf changed the title Handle acting on Ethereum events that take longer than one round to confirm Handle acting on tallied pieces of data that take longer than one round to confirm Nov 10, 2022
@james-chf james-chf force-pushed the james/ethbridge/multiple-votes branch 2 times, most recently from b7ab3ec to b35434f Compare November 14, 2022 11:38
@james-chf james-chf force-pushed the james/ethbridge/multiple-votes branch from 4c97319 to 362a340 Compare November 21, 2022 11:01
@james-chf james-chf requested review from batconjurer and sug0 and removed request for batconjurer November 24, 2022 13:29
@james-chf james-chf marked this pull request as ready for review November 24, 2022 13:29
shared/src/ledger/protocol/transactions/votes/update.rs Outdated Show resolved Hide resolved
shared/src/ledger/protocol/transactions/votes/update.rs Outdated Show resolved Hide resolved
tracing::info!(
?keys.prefix,
validators = ?vote_info.voters(),
"Recording validators as having voted for this event"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also called from validator set update code, right? maybe we should change event to something else?

Suggested change
"Recording validators as having voted for this event"
"Recording validators as having voted for this XXX"

but I'm not sure what to replace XXX with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I am not sure of a good term either. I'm updating this log to "Calculating validators' votes applied to an existing tally"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps you could attach a context message to T, something like T: TallyItem, or whatever, that returned a static string identifying itself

"Recording validators as having voted for this event"
);
let tally_pre = super::storage::read(store, keys)?;
if tally_pre.seen {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we change seen to something like approved? and seen_by to something along the lines of voted_by. this is a bit more generic. I've noticed seen and seen_by in the logs for validator set updates, and thought it was a bit silly, since it makes more sense in the context of ethereum events, rather than valset upds

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course this would only apply to a future PR, not this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯% agree, the term "seen" is overloaded in our design/code.

shared/src/ledger/protocol/transactions/votes/update.rs Outdated Show resolved Hide resolved
shared/src/ledger/protocol/transactions/votes/update.rs Outdated Show resolved Hide resolved
shared/src/ledger/protocol/transactions/votes/update.rs Outdated Show resolved Hide resolved
@james-chf james-chf marked this pull request as draft November 24, 2022 14:23
@james-chf james-chf marked this pull request as ready for review November 24, 2022 17:10
@james-chf james-chf force-pushed the james/ethbridge/multiple-votes branch from ef2a9b6 to c0654d8 Compare November 28, 2022 15:00
@james-chf
Copy link
Contributor Author

Have force pushed a rebase onto eth-bridge-integration (there were no merge conflicts), to run CI with the latest namada v0.10.1-based code

@james-chf james-chf marked this pull request as draft December 7, 2022 10:57
@james-chf james-chf marked this pull request as ready for review December 7, 2022 13:35
@james-chf james-chf requested a review from batconjurer December 7, 2022 13:35
@james-chf james-chf merged commit c776aaf into eth-bridge-integration Dec 8, 2022
@james-chf james-chf deleted the james/ethbridge/multiple-votes branch December 8, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process Ethereum event vote extensions correctly for chains with more than one validator
4 participants