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

feat: solana backoff deposit witnessing #5636

Merged
merged 12 commits into from
Feb 18, 2025

Conversation

kylezs
Copy link
Contributor

@kylezs kylezs commented Feb 13, 2025

Pull Request

Closes: PRO-1984

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

This will drastically reduce the number of RPC requests related to deposit witnessing. We can probably do something similar for egress witnessing too.

@kylezs kylezs force-pushed the feat/solana-backoff-deposit-witnessing branch from 81b0ef1 to 1fb1070 Compare February 13, 2025 15:34
Copy link
Contributor

@marcellorigotti marcellorigotti left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@MxmUrw MxmUrw left a comment

Choose a reason for hiding this comment

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

Looking good! One question for my understanding: are we sure that the engines are voting for each statechain block?

Comment on lines 204 to 207
Ok(!((current_state_chain_block_number.clone() >
last_channel_opened_at + backoff_settings.backoff_after_blocks) &&
(current_state_chain_block_number.clone() % backoff_settings.backoff_frequency !=
Zero::zero())))
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be more readable if we apply the negation to the clauses:
(current <= opened + backoff) || (current % freq == zero)

@kylezs
Copy link
Contributor Author

kylezs commented Feb 14, 2025

Looking good! One question for my understanding: are we sure that the engines are voting for each statechain block?

Good question. The answer is no. You're asking because when we backoff, we have to have got the elections at that particular block in order for the mod = 0 condition to be true? ... I thought about it, and figured given this is a kinda of backup case anyway, as in, we assume we deposit in the first hour anyway, then a potential added 10 minute delay isn't that bad. Plus, it's quite unlikely we hit this case given we query every 6 seconds anyway.

@marcellorigotti
Copy link
Contributor

Looking good! One question for my understanding: are we sure that the engines are voting for each statechain block?

Good question. The answer is no. You're asking because when we backoff, we have to have got the elections at that particular block in order for the mod = 0 condition to be true?

Why would we miss an election at a particular block? isn't the engine processing all the state-chain blocks? 🤔

@kylezs
Copy link
Contributor Author

kylezs commented Feb 14, 2025

Why would we miss an election at a particular block? isn't the engine processing all the state-chain blocks? 🤔

You're right actually. Was thinking about submission. But for processing of the data we use the unfinalised stream, so even better :)

engine/src/elections.rs

if let Some(block_info) = unfinalized_block_stream.next() => {

@kylezs kylezs force-pushed the feat/solana-backoff-deposit-witnessing branch from d086341 to 333cbfa Compare February 14, 2025 10:58
@MxmUrw
Copy link
Contributor

MxmUrw commented Feb 14, 2025

You're right actually. Was thinking about submission. But for processing of the data we use the unfinalised stream, so even better :)

I'm not sure whether this changes things, if we start processing the data for some block with height % freq = 0, but aren't fast enough to submit it, then the next chance will be in 10 minutes anyways, right?

[...] then a potential added 10 minute delay isn't that bad.

I agree with this though.

@kylezs kylezs force-pushed the feat/solana-backoff-deposit-witnessing branch from 9ff3436 to 00eb5fa Compare February 14, 2025 14:56
@kylezs kylezs force-pushed the feat/solana-backoff-deposit-witnessing branch from 00eb5fa to 4f151cb Compare February 14, 2025 14:58
@kylezs kylezs changed the title WIP: feat: solana backoff deposit witnessing feat: solana backoff deposit witnessing Feb 17, 2025
@kylezs kylezs force-pushed the feat/solana-backoff-deposit-witnessing branch from 336d7a0 to cda1157 Compare February 17, 2025 08:37
@kylezs kylezs marked this pull request as ready for review February 17, 2025 09:23
@kylezs kylezs requested a review from dandanlen as a code owner February 17, 2025 09:23
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

I feel like there's something a little leaky about the abstraction here... maybe there's a better way to tackle this?

(Not saying I know what it is, just that something doesn't feel quite right here)

Basically, it feels like this is neither encapsulated in the abstraction layer of electoral systems, nor fully hidden inside the implementation of the election. It sort of exists in both, but is not useful in either piece individually.

@kylezs kylezs added this pull request to the merge queue Feb 18, 2025
Merged via the queue into main with commit b354005 Feb 18, 2025
48 checks passed
@kylezs kylezs deleted the feat/solana-backoff-deposit-witnessing branch February 18, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants