Avoid races in system_tests changing config by making the changes atomic #3829
+128
−80
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
ganeshvanahalli
requested review from
MishkaRogachev,
magicxyyz and
pmikolajczyk41
Comment on lines
577
to
+578
| builder.nodeConfig.BatchPoster.MaxDelay = 0 | ||
| builder.L2.ConsensusConfigFetcher.Set(builder.nodeConfig) |
There was a problem hiding this comment.
given that Set always occurs in this pattern: (1) change config (2) set config, we might actually change the fetcher API from Set() to Update(func (*T))
There was a problem hiding this comment.
for some reason git didnt post my reply to your comment! my bad.
Regarding Update instead of Set I think Set makes more sense like suggested in the ticket https://linear.app/offchain-labs/issue/NIT-3815/system-tests-avoid-races-for-tests-changing-config because what does Update even mean? does it mean we call func on the existing config var stored in the atomic pointer? realize that we arent using mutexs here (to make the process of changing configs fast and efficient we use atomic.Pointer) so changing that config basically means introducing race again.
There was a problem hiding this comment.
nah, the intention was to perform field reassignments on some temp copy and then use atomic.Pointer as we currently do
my motivation was to wrap the 2-instruction pattern within the fetcher API, but maybe it doesn't make that much sense here; waiving
There was a problem hiding this comment.
I maintain my suggestion about wrapping the pattern with Update instead of Set; wdyt @gligneul ?
pmikolajczyk41
requested changes
Oct 17, 2025
There was a problem hiding this comment.
given that @gligneul agrees, I would convert my suggestion with change request - please convert this pattern:
config.field = value
actual_config.Set(config)
with
actual_config.Update(func (config) {
config.field = value
})
it should make us a bit safer
I dont know if you read my response here #3829 (comment), |
pmikolajczyk41
approved these changes
Oct 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously system_tests used
NodeBuilder'snodeConfigandexecConfigto create consensus and execution nodes directly, meaning if the test wanted to change any config fields- it would end up changing behavior of the nodes already started as well, sometimes unintentionally! As theNodeBuilder's configs are to be used for building multiple nodes with different configurations.To facilitate this we introduce
commonConfigFetcherthat can take in both consensus and execution config- clone the config and store the new config atomically in its localatomic.Pointerfiled and return aConfigFetcherinterface implemented by both kinds of nodes. And tests planning to changing the config fields after the node has started should use respective configfetchers of consensus and execution nodes available in theTestClientobject to make changes and useSet()method to update the configs atomically.Resolves NIT-3815