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: add canPruneAtTime #9751

Merged
merged 4 commits into from
Nov 7, 2024
Merged

feat: add canPruneAtTime #9751

merged 4 commits into from
Nov 7, 2024

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Nov 5, 2024

A minor change to help on #9308. Instead of the archiver using the current time, it will use the time of the next ethereum block.

Also addresses an issue, where it would be possible to submit a proof for blocks that should have been pruned, but have not yet been because of no new publications.

@LHerskind LHerskind marked this pull request as ready for review November 5, 2024 13:19
@LHerskind LHerskind linked an issue Nov 5, 2024 that may be closed by this pull request
@just-mitch
Copy link
Contributor

I thought about making this change here when I added it to the quote validation stuff, but had a concern:

Suppose you're in the beginning of slot 12 of an epoch, and no proof quote has been claimed. Doesn't this change make it so that if a proof does land after I look, I've already unwound?

But considering further, even if that does happen, it should just rebuild the chain, so maybe it is fine?

Regardless, it almost seems like the current behavior is the correct one?

@LHerskind
Copy link
Contributor Author

LHerskind commented Nov 6, 2024

I thought about making this change here when I added it to the quote validation stuff, but had a concern:

Suppose you're in the beginning of slot 12 of an epoch, and no proof quote has been claimed. Doesn't this change make it so that if a proof does land after I look, I've already unwound?

But considering further, even if that does happen, it should just rebuild the chain, so maybe it is fine?

Regardless, it almost seems like the current behavior is the correct one?

I don't get what you say? If it lands after you looked, it landed after it should be pruned. There should be no time for a proof to land in between you looking and it pruning?

If we can do that, it is an inconsistency in the flow, e.g., a bug. Looking at submitEpochRootProof looks like it do not check at all if it should be pruned, which just seems plain wrong to me. If it is to "automatically" prune, it should always do it, any action that moves the chain any way should perform the check.

This nicely shows why #7868 is needed. Doing the change breaks 0 tests, so something is missing. Some of the tests also state they check pruning but don't actually do any pruning.

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Changes to public function bytecode sizes

Generated at commit: 3a8f209c5ce87e89f149a8510a3b671e1ca11493, compared to commit: d671af6cbaca7f644187cc3b110a93bbc3b24b14

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
AvmTest::public_dispatch +851 ❌ +1.43%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
AvmTest::public_dispatch 60,539 (+851) +1.43%

Base automatically changed from lh/9362 to master November 7, 2024 13:06
@LHerskind LHerskind merged commit 0cb0343 into master Nov 7, 2024
70 checks passed
@LHerskind LHerskind deleted the lh/9308-can-prune-at branch November 7, 2024 14:01
ludamad pushed a commit that referenced this pull request Nov 7, 2024
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.

bug: canPrune should take a time
3 participants