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

ci: wait for 3 blocks before starting contract #10295

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

rabi-siddique
Copy link
Contributor

@rabi-siddique rabi-siddique commented Oct 18, 2024

closes: #9325

Description

As per my inspection, the initial blocks often contain transactions from governance-related wallets when the chain starts. Starting the contract also involves these governance wallets. When these transactions overlap during the initial blocks, it can lead to an account sequence mismatch. Since we now wait for three blocks to be produced, this should be sufficient to surpass the phase where governance wallets are involved in transactions.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@rabi-siddique rabi-siddique added the force:integration Force integration tests to run on PR label Oct 18, 2024
Copy link

cloudflare-workers-and-pages bot commented Oct 18, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 10ee3d4
Status: ✅  Deploy successful!
Preview URL: https://8a410b03.agoric-sdk.pages.dev
Branch Preview URL: https://rs-fix-getting-started-flake.agoric-sdk.pages.dev

View logs

@rabi-siddique rabi-siddique changed the title chore: wait for 1000 blocks chore: wait for 2 blocks before starting contract Oct 18, 2024
@mujahidkay mujahidkay changed the title chore: wait for 2 blocks before starting contract ci: wait for 2 blocks before starting contract Oct 18, 2024
@mujahidkay mujahidkay changed the title ci: wait for 2 blocks before starting contract ci: wait for 3 blocks before starting contract Oct 18, 2024
@rabi-siddique rabi-siddique marked this pull request as ready for review October 18, 2024 12:11
@rabi-siddique rabi-siddique requested a review from a team as a code owner October 18, 2024 12:11
if (earlierHeight > 0n && currentHeight > earlierHeight) {
// We've had at least one block produced.
if (earlierHeight > 2n && currentHeight > earlierHeight) {
// We've had at least 3 blocks produced.
Copy link
Contributor

Choose a reason for hiding this comment

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

How is waiting for three blocks fix the account seq mismatch issue? Isn't it due to parallel jobs doing tx with same account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is waiting for three blocks fix the account seq mismatch issue?

As per my inspection, the initial blocks often contain transactions from governance-related wallets when the chain starts. Starting the contract also involves these governance wallets. When these transactions overlap during the initial blocks, it can lead to an account sequence mismatch. Since we now wait for three blocks to be produced, this should be sufficient to surpass the phase where governance wallets are involved in transactions.

Isn't it due to parallel jobs doing tx with same account?

No. Each getting-started job runs independently in its own runner. There's no shared state between workflows, as each one runs its own A3P chain in a separate space.

Copy link
Contributor

@Muneeb147 Muneeb147 Oct 18, 2024

Choose a reason for hiding this comment

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

Hmm right. So As far as I know, this tx sequence is linked with account/wallet as well. Our tests use user1 (install-bundles.sh), so about governance wallet transactions, their sequence number shouldn't disturb user1, right? Not sure just thnking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user1 is involved in installing bundles. But for the contract core eval to be passed, I reckon governance wallets take part in it via voting on it. I think that phase leads to account sequence mismatch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm interesting.
So do we have successful re-runs as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Mujahid and I ran it several times. Did not encounter any flakes or failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Muneeb147 Muneeb147 left a comment

Choose a reason for hiding this comment

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

Good possible detection.
Can you also update PR description with
#10295 (comment)?

Also I see we did multiple runs with successful outcome 🤞❤️

@rabi-siddique rabi-siddique added the automerge:rebase Automatically rebase updates, then merge label Oct 18, 2024
@rabi-siddique rabi-siddique merged commit 2ca7ea6 into master Oct 18, 2024
103 of 104 checks passed
@rabi-siddique rabi-siddique deleted the rs-fix-getting-started-flakes branch October 18, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flake: Getting Started (registry/yarn)
3 participants