tests: 🏃 mock consensus can exercise staking component #4001

cratelyn · 2024-03-12T00:06:51Z

fixes #3966. fixes #3908. fixes part of #3995.

this branch introduces the first steps towards mock consensus (#3588) testing of the staking component (#3845).

this defines a validator after genesis, and then shows that it does not enter the consensus set. #3966 is addressed in this branch so that the existing genesis validator correctly enters the consensus set, and so that we can successfully progress to the second epoch.

subsequent changes will exercise delegating to this validator in the mock_consensus_can_define_and_delegate_to_a_validator.

✨ changes

alters with_penumbra_auto_app_state so that it adds an allocation of delegation tokens to the shielded pool component's genesis content.
extends generate_penumbra_validator so that it generates a real spend key, and returns an Allocation for the generated validator. (see mock-consensus: 🪙 auto_app_state assigns delegation tokens #3966)
adds a new mock_consensus_can_define_and_delegate_to_a_validator test that defines a post-genesis validator. (see tests: 🐦 mock consensus supports post-genesis validators #3908)
defines a new ConsensusIndexRead::get_consensus_set() method, which collects all of the identity keys returned by consensus_set_stream.
lowers the events in penumbra_mock_consensus::block::Builder::execute() to trace-level events.
penumbra_mock_consensus::builder::Builder will now log a warning if values may be errantly rewritten by the builder methods.
TestNode::fast_forward sets its i span field to 1..n, rather than 0..n-1.

🔗 related

cratelyn · 2024-03-12T17:35:50Z

cherry-picked logging additions out into #4009.

fixes a few comments in `App::end_block`. cherry-picked from #4001.

`Consensus::end_block` never returns an `Err(_)` value, so it does not need to return a `Result<T, E>`. cherry-picked out ouf #4001.

this is a group of improvements to the telemetry in the staking component, cherry-picked out of #4001. this improves the logs seen in tests, and additionally takes the time to make some other small gardening tweaks: consolidating imports, adding documentation comments, etc. this is another branch in the spirit of #4009. the changes here are itemized into distinct, small code transformations to facilitate review, and demonstrate that these are noop changes. you can start [here](f7f78b9). * f331964 - feat(staking): ❗ traces for invalid or missing parameters * d33598d - docs(staking): 🤝 add a doc comment * f6c60a8 - staking: 🙂 remove needless `Ok(_)` * fd4b8da - feat(staking): 💬 log when delegation changes are not found * fe597ff - feat(staking): 🔍 trace-level spans for `StateReadExt` methods * 0447600 - refactor(staking): 🚡 use `self::` prefix in reexports * 67893c9 - refactor(staking): 🙂 remove import from reexports * 043222e - refactor(staking): 🎁 consolidate public submodules * 6376796 - refactor(staking): 🎁 consolidate submodules * b5bd240 - refactor(staking): 👯 component flag reexports StateWriteExt * d57beaa - chore(staking): 🥡 consolidate component reexports * aa52f6d - chore(staking): 🧹 tidy epoch_handler imports * 765ed75 - refactor(compact-block): 🧹 tidy imports * 7cb93f7 - feat(staking): 🌐 log why epoch ended * 081d041 - refactor(staking): add context to `state.end_epoch` call * 5701295 - feat(app): 🦠 tracing telemetry for `App::end_block` * f7f78b9 - docs(staking): 📕 add doc comment for `end_epoch`

crates/core/app/tests/mock_consensus_staking.rs

https://github.com/penumbra-zone/penumbra/pull/4001/files#r1527344233

https://github.com/penumbra-zone/penumbra/pull/4001/files#r1527343587

cratelyn

i've cherry-picked changes from staking-mock-consensus-tweaks into this branch. it resolves most of the comments in discussion above, and i've pushed a6584c6, 733b9a1, and 9f569c4 to address the other remaining threads.

crates/core/app/tests/mock_consensus_staking.rs

crates/core/app/tests/mock_consensus.rs

crates/core/app/tests/mock_consensus_staking.rs

crates/core/app/tests/common/test_node_builder_ext.rs

cratelyn · 2024-03-18T14:08:57Z

crates/core/app/tests/mock_consensus_staking.rs

+ let (new_validator_pb, _, new_validator_spend_key) =
+ self::common::generate_penumbra_validator(&new_validator_vk);


we don't want to be using a common helper method to define the new validator we're adding. It should be possible to reason about the semantics of the test function just from reading its code, but if we reuse an internal helper method of the auto_app_state implementation, we're coupling the semantics of the test to the implementation of some other part of the test harness. This is actively undesirable.

definitely. i agree. it was a mistake to reuse that helper function here. i have cherry-picked ceb5d62 into this branch.

crates/core/app/tests/common/test_node_builder_ext.rs

previously, we added a `Keyring` newtype. this was a low-effort, thin newtype wrapper around a `BTreeMap<VerificationKey, SigningKey>`. this commit removes it. NB: this is based on #4001.

cratelyn · 2024-03-18T15:46:51Z

opened #4039 to remove the keyring submodule, using a type alias rather than a newtype, and moving key generation logic out to call-sites.

cratelyn · 2024-03-18T19:57:01Z

crates/core/app/tests/mock_consensus_staking.rs

+ // TODO: upstream a direct conversion from ed25519_consensus
+ consensus_key: tendermint::PublicKey::from_raw_ed25519(&new_validator_consensus.to_bytes())
+ .expect("consensus key is valid"),


filed informalsystems/tendermint-rs#1401 to address this TODO 🙂

### 🚠 overview in #4001 we added a test for the staking component (_see also #3995, and #3588_). this shortens the epoch length, so that this test runs in less time, and importantly, so that debugging test failures does not entail sifting through many log lines. ### ➕ changes * manually construct an `AppState` so that we use a shorter epoch. this test involves waiting for multiple epochs to pass, which takes many seconds with the default length of 719, see: <https://github.com/penumbra-zone/penumbra/blob/8be8e8266b312c2a9eb1b1522a937e8df12f3a4f/crates/core/component/sct/src/params.rs#L41> * `TestNode<C>::fast_forward` now accepts a u64, mirroring the type used in the sct parameters. ### 🏃 runtime on `main`... ``` Starting 1 test across 1 binary PASS [ 12.646s] penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator ------------ Summary [ 12.647s] 1 test run: 1 passed, 0 skipped ``` after these changes... ``` Starting 1 test across 1 binary PASS [ 0.420s] penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator ------------ Summary [ 0.420s] 1 test run: 1 passed, 0 skipped ``` ### 🥙 log volume on `main`... ``` ; RUST_LOG='info' cargo nextest run -p penumbra-app --test mock_consensus_staking --no-capture | wc -l Finished test [unoptimized + debuginfo] target(s) in 0.16s Starting 1 test across 1 binary START penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator PASS [ 12.185s] penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator ------------ Summary [ 12.185s] 1 test run: 1 passed, 0 skipped 46082 ``` ``` ; RUST_LOG='info' cargo nextest run -p penumbra-app --test mock_consensus_staking --no-capture | wc -l Finished test [unoptimized + debuginfo] target(s) in 0.15s Starting 1 test across 1 binary START penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator PASS [ 0.416s] penumbra-app::mock_consensus_staking mock_consensus_can_define_and_delegate_to_a_validator ------------ Summary [ 0.416s] 1 test run: 1 passed, 0 skipped 450 ```

cratelyn added the A-mock-consensus Area: Relates to the mock consensus engine label Mar 12, 2024

cratelyn added this to the Sprint 2 milestone Mar 12, 2024

cratelyn self-assigned this Mar 12, 2024

cratelyn temporarily deployed to smoke-test March 12, 2024 00:06 — with GitHub Actions Inactive

cratelyn mentioned this pull request Mar 12, 2024

mock-consensus: 🍐 clarify builder kind in error #4003

Closed

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from f66d974 to cb15f6d Compare March 12, 2024 00:41

cratelyn temporarily deployed to smoke-test March 12, 2024 00:41 — with GitHub Actions Inactive

cratelyn mentioned this pull request Mar 12, 2024

Tracking issue: 🤝 mock consensus engine for App tests #3588

Closed

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from cb15f6d to 1a1e3cf Compare March 12, 2024 17:24

cratelyn had a problem deploying to smoke-test March 12, 2024 17:24 — with GitHub Actions Error

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from 1a1e3cf to e25db4c Compare March 12, 2024 17:38

cratelyn temporarily deployed to smoke-test March 12, 2024 17:38 — with GitHub Actions Inactive

This was referenced Mar 12, 2024

refactor(staking): 😊 tracing telemetry for delegation changes #4009

Merged

app: 🤓 fix grammar in comment #4010

Merged

app: 🦅 Consensus::end_block never errors #4011

Merged

cratelyn added a commit that referenced this pull request Mar 12, 2024

app: 🤓 tidy propsal validation comments (#4010)

e5a0a53

fixes a few comments in `App::end_block`. cherry-picked from #4001.

cratelyn added a commit that referenced this pull request Mar 12, 2024

app: 🦅 Consensus::end_block never errors (#4011)

03444d4

`Consensus::end_block` never returns an `Err(_)` value, so it does not need to return a `Result<T, E>`. cherry-picked out ouf #4001.

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from e25db4c to bb90f30 Compare March 12, 2024 20:50

cratelyn temporarily deployed to smoke-test March 12, 2024 20:50 — with GitHub Actions Inactive

cratelyn mentioned this pull request Mar 12, 2024

staking: use block height to enforce unbonding delay #3923

Merged

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from bb90f30 to c601f21 Compare March 12, 2024 21:54

cratelyn temporarily deployed to smoke-test March 12, 2024 21:54 — with GitHub Actions Inactive

cratelyn temporarily deployed to smoke-test March 12, 2024 22:24 — with GitHub Actions Inactive

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from 6bf52ba to cd5950d Compare March 13, 2024 17:36

cratelyn temporarily deployed to smoke-test March 13, 2024 17:36 — with GitHub Actions Inactive

cratelyn mentioned this pull request Mar 13, 2024

staking: 🎁 add additional tracing telemetry #4017

Merged

cratelyn force-pushed the kate/mock-consensus-can-exercise-staking-component branch from cd5950d to bd087ce Compare March 13, 2024 18:22

cratelyn temporarily deployed to smoke-test March 13, 2024 18:22 — with GitHub Actions Inactive

hdevalence reviewed Mar 16, 2024

View reviewed changes

crates/core/app/tests/mock_consensus_staking.rs Outdated Show resolved Hide resolved

decouple test from auto_app_state code

db3d1df

cratelyn had a problem deploying to smoke-test March 18, 2024 13:49 — with GitHub Actions Error

remove #[ignore] test attribute

9f569c4

cratelyn had a problem deploying to smoke-test March 18, 2024 13:51 — with GitHub Actions Error

staking: promote get_consensus_set to ConsensusIndexRead

733b9a1

https://github.com/penumbra-zone/penumbra/pull/4001/files#r1527344233

cratelyn had a problem deploying to smoke-test March 18, 2024 13:59 — with GitHub Actions Error

tests(app): rename test

a6584c6

https://github.com/penumbra-zone/penumbra/pull/4001/files#r1527343587

cratelyn temporarily deployed to smoke-test March 18, 2024 14:01 — with GitHub Actions Inactive

cratelyn commented Mar 18, 2024

View reviewed changes

tests(app): do not return spend key from generate_penumbra_validator

3ca8c46

cratelyn temporarily deployed to smoke-test March 18, 2024 14:23 — with GitHub Actions Inactive

cratelyn mentioned this pull request Mar 18, 2024

refactor(mock-consensus): 🔐 remove Keyring #4039

Merged

cratelyn marked this pull request as ready for review March 18, 2024 15:07

cratelyn mentioned this pull request Mar 18, 2024

tests: 🧨 mock consensus exercises staking component #3995

Closed

26 tasks

cratelyn merged commit 90be256 into main Mar 18, 2024
6 checks passed

cratelyn deleted the kate/mock-consensus-can-exercise-staking-component branch March 18, 2024 15:26

cratelyn modified the milestones: Sprint 2, Sprint 3 Mar 18, 2024

cratelyn modified the milestones: Sprint 3, Sprint 2 Mar 18, 2024

cratelyn commented Mar 18, 2024

View reviewed changes

cratelyn mentioned this pull request Mar 19, 2024

tests(app): ⌛ shorten epoch length in staking test #4048

Merged

cratelyn referenced this pull request Mar 19, 2024

decouple test from auto_app_state code

ceb5d62

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: 🏃 mock consensus can exercise staking component #4001

tests: 🏃 mock consensus can exercise staking component #4001

cratelyn commented Mar 12, 2024 •

edited

Loading

cratelyn commented Mar 12, 2024

cratelyn left a comment

cratelyn Mar 18, 2024

cratelyn commented Mar 18, 2024

cratelyn Mar 18, 2024

		let (new_validator_pb, _, new_validator_spend_key) =
		self::common::generate_penumbra_validator(&new_validator_vk);

tests: 🏃 mock consensus can exercise staking component #4001

tests: 🏃 mock consensus can exercise staking component #4001

Conversation

cratelyn commented Mar 12, 2024 • edited Loading

✨ changes

🔗 related

cratelyn commented Mar 12, 2024

cratelyn left a comment

Choose a reason for hiding this comment

cratelyn Mar 18, 2024

Choose a reason for hiding this comment

cratelyn commented Mar 18, 2024

cratelyn Mar 18, 2024

Choose a reason for hiding this comment

cratelyn commented Mar 12, 2024 •

edited

Loading