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

Fix issue with modifySorobanNetworkConfig #4583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SirTyson
Copy link
Contributor

Description

Resolves an issue with modifySorobanNetworkConfig, where sometimes bucket levels would merge multiple times. Note meta has changed as some tests now close an additional ledger during modifySorobanNetworkConfig.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@SirTyson SirTyson marked this pull request as ready for review December 18, 2024 00:58
@SirTyson SirTyson requested review from sisuresh and dmkozh December 18, 2024 00:58
sisuresh
sisuresh previously approved these changes Dec 18, 2024
@SirTyson SirTyson added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@SirTyson SirTyson enabled auto-merge December 19, 2024 01:25
@marta-lokhova
Copy link
Contributor

Can we force externalize + close ledger once with network config upgrades instead of what we're doing right now? I've been debugging some test failures in #4597, and it seems related, although I don't exactly understand what's going on in this flow. Right now we add batch and update snapshot, followed by an empty ledger close, which again adds batch and updates the snapshot. That feels quite error-prone. Why do we need to close an extra empty ledger?

@marta-lokhova
Copy link
Contributor

took a while to figure out what's going on, but looks like ledger close meta in tests was broken because of double add batch (I updated the meta in 25e8023). When we manually add batch, we update snapshots in tests, but then when we actually close ledger (same ledger seq), we do not update snapshot because of this condition.

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.

3 participants