Update genesis processing to have a fallback collector id for tests #34135

jstarry · 2023-11-17T06:39:31Z

Problem

When creating bank 0 for tests, oftentimes the genesis config doesn't include a staked node. This means that bank creation has to arbitrarily pick a node id for block 0 in order for bank creation to succeed. The arbitrary node id was recently updated to be a random unique pubkey in #33887 but as discussed here we need to make sure this path is never hit by tests.

Summary of Changes

Add the arg test_collector_id: Option<Pubkey> to Bank::new_with_paths which can be used as a fallback collector id if no staked nodes are defined in the genesis. This arg is set to None in call-sites that aren't related to testing or benchmarks.

Fixes #

codecov · 2023-12-18T11:10:31Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b814497) 81.8% compared to head (258244d) 81.8%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34135   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222394   222405   +11     
=======================================
+ Hits       181957   181973   +16     
+ Misses      40437    40432    -5

t-nelson · 2023-12-20T23:33:49Z

is there something stopping us from fixing up the GenesisConfig instantiation to always include at least one staked node? seems less brittle

jstarry · 2023-12-21T01:18:15Z

is there something stopping us from fixing up the GenesisConfig instantiation to always include at least one staked node? seems less brittle

That was my initial approach but it was too unruly. One thing I ran into is that it should still be possible to create a GenesisConfig without staked nodes if you want to unit test the struct and don't instantiate a bank with it. This leaves open constructor methods that can be misused. And many of the tests that need a basic bank don't care about the collector id or staked nodes at all. So I was doing a lot of work to refactor a million tests that really just needed a placeholder collector id. Since test_collector_id can only be set by test methods, this seemed the most sane way.

runtime/src/bank.rs

brooksprumo · 2023-12-21T14:29:29Z

runtime/src/bank.rs

        self.collector_id = self
            .stakes_cache
            .stakes()
            .highest_staked_node()
-            .unwrap_or_else(Pubkey::new_unique);
+            .or(test_collector_id)


Would it be worse to create-and-store this collector id instead? (And maybe manually add it to stakes cache too.) Then it would actually be a proper stake account.

The collector id is the "node id" but yeah we could create a stake account and vote account which delegate stake to the collector id. I'll play around with this.. just wary of breaking dozens of tests with brittle assumptions

[..] wary of breaking dozens of tests with brittle assumptions

I definitely understand this...

Thinking about this more, wouldn't it be strange if extra accounts were stored that weren't defined in the genesis?

is a genesis with no collector a reasonable thing? barring tests that don't strictly need one, that is. this is almost certainly yet another case of data structure abuse that would justify refactor

I would like to just add a genesis_collector_id to the genesis but we can't make backwards incompatible changes to that struct

we can make a wrapper for tests and a trait though, right?

Yeah, we can. It involves updating hundreds of tests and I don't have the time for that.

t-nelson

just gross enough to prevent overuse and encourage a refactor 👍

* wrap test_collector_id in DCOU * rename param to collector_id_for_tests

jstarry · 2024-01-09T02:09:05Z

I'd like to merge as is, I would also like to this be a bit cleaner but cleaning up the tests is too much of a burden and it's inconvenient that genesis can't be modified to specify an initial collector.

brooksprumo

Looks good to me (but I'm biased a bit). One open question on my end.

brooksprumo · 2024-01-09T03:41:29Z

runtime/src/bank.rs

@@ -1041,7 +1041,7 @@ impl Bank {
        debug_do_not_add_builtins: bool,
        accounts_db_config: Option<AccountsDbConfig>,
        accounts_update_notifier: Option<AccountsUpdateNotifier>,
-        #[cfg(feature = "dev-context-only-utils")] collector_id_for_tests: Option<Pubkey>,
+        #[allow(unused)] collector_id_for_tests: Option<Pubkey>,


I guess something in the build didn't like marking this parameter as DCOU?

Yeah, there was a call-site that didn't have DCOU handling and then I just figured it was easier to handle internally. What do you think? Is it confusing that the parameter is ignored?

Yeah, there was a call-site that didn't have DCOU handling and then I just figured it was easier to handle internally.

Gotcha, thanks.

What do you think? Is it confusing that the parameter is ignored?

I think it's a little confusing, but... the whole codebase is a little confusing, heh. So doesn't seem too confusing IMO.

"easier" is not really what we should be optimizing for at this point in the project's maturity

Yeah "easier" is not the best word to use there. The main issue is that Rust doesn't let you conditionally add parameters to a function call and so it means duplicating the call site which is a worse dev experience than having an unused parameter.

jstarry mentioned this pull request Nov 17, 2023

Burn fees collected into invalid accounts #33887

Merged

jstarry force-pushed the test/cleanup-highest-stake branch from 9a0b007 to 0c51e1b Compare November 17, 2023 06:49

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 1, 2023

jstarry force-pushed the test/cleanup-highest-stake branch 4 times, most recently from d1f78fa to c89ddda Compare December 8, 2023 09:19

jstarry marked this pull request as ready for review December 8, 2023 09:19

jstarry removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 8, 2023

jstarry marked this pull request as draft December 8, 2023 13:44

jstarry force-pushed the test/cleanup-highest-stake branch from c89ddda to 313438e Compare December 8, 2023 14:46

jstarry changed the title ~~test cleanup: remove usages of incomplete genesis creation~~ Update genesis processing to have a fallback collector id for tests Dec 8, 2023

jstarry marked this pull request as ready for review December 8, 2023 14:49

jstarry force-pushed the test/cleanup-highest-stake branch from 313438e to 6f3a7d3 Compare December 18, 2023 09:51

jstarry requested a review from t-nelson December 18, 2023 13:20

jstarry requested a review from brooksprumo December 21, 2023 01:21

jstarry force-pushed the test/cleanup-highest-stake branch from 8146144 to 7388d39 Compare December 21, 2023 01:23

brooksprumo reviewed Dec 21, 2023

View reviewed changes

t-nelson previously approved these changes Jan 4, 2024

View reviewed changes

jstarry and others added 2 commits January 5, 2024 16:36

Update genesis processing to have a fallback collector id for tests

6a51eba

DCOU-ify the collector id for tests parameter (#1902)

00ef349

* wrap test_collector_id in DCOU * rename param to collector_id_for_tests

jstarry dismissed t-nelson’s stale review via 00ef349 January 5, 2024 08:42

jstarry force-pushed the test/cleanup-highest-stake branch from efcb085 to 00ef349 Compare January 5, 2024 08:42

jstarry added 2 commits January 5, 2024 17:01

fix program test

b07b265

fix dcou

258244d

brooksprumo reviewed Jan 9, 2024

View reviewed changes

brooksprumo approved these changes Jan 9, 2024

View reviewed changes

jstarry merged commit 5f74fc4 into solana-labs:master Jan 10, 2024
35 checks passed

jstarry deleted the test/cleanup-highest-stake branch January 10, 2024 00:34

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update genesis processing to have a fallback collector id for tests #34135

Update genesis processing to have a fallback collector id for tests #34135

jstarry commented Nov 17, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

t-nelson commented Dec 20, 2023

jstarry commented Dec 21, 2023 •

edited

Loading

brooksprumo Dec 21, 2023

jstarry Jan 2, 2024

brooksprumo Jan 3, 2024

jstarry Jan 5, 2024

t-nelson Jan 5, 2024

jstarry Jan 5, 2024

t-nelson Jan 9, 2024

jstarry Jan 10, 2024

t-nelson left a comment

jstarry commented Jan 9, 2024

brooksprumo left a comment

brooksprumo Jan 9, 2024

jstarry Jan 9, 2024

brooksprumo Jan 9, 2024

t-nelson Jan 9, 2024

jstarry Jan 10, 2024

Update genesis processing to have a fallback collector id for tests #34135

Update genesis processing to have a fallback collector id for tests #34135

Conversation

jstarry commented Nov 17, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Dec 18, 2023 • edited Loading

Codecov Report

t-nelson commented Dec 20, 2023

jstarry commented Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

jstarry commented Jan 9, 2024

brooksprumo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Nov 17, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

jstarry commented Dec 21, 2023 •

edited

Loading