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

cratelyn · 2024-01-08T16:08:58Z

💭 background and motivation

currently, writing complete end-to-end integration tests for Penumbra is
difficult. moreover, we don't have a better alternative strategy for testing the core App type.

currently, the smoke-test.sh script performs the steps needed
to run integration tests of the system, including:

generate testnet configuration
start a cometbft process
start a pd process
run pclientd integration tests against the local test network
run pcli integration tests against the local test network
confirm the test network is still running

because these integration tests assume a test network including pd and
cometbft is running locally, these tests must run serially, and must spend
time waiting for new blocks to be generated.

additionally, this means we must under-handedly make use of the #[ignore]
attribute to prevent these tests from running (and failing) alongside unit
tests when commands like cargo test --workspace are run.

see CometMock for a mock implementation of CometBFT implemented
in Go. see #2771 for previous work in this space integrating with CometMock.

📐 overview

this state of affairs would be improved if we had a mock implementation of the
consensus engine (CometBFT) that would allow #[test] functions to
generate ABCI messages. this mock implementation should function as a drop-in
replacement for CometBFT from the perspective of the pd daemon.

this mocking library should additionally expose some "meta" interfaces. to allow us
to write integration tests including things like:

manipulating the timer used for generating blocks, so that edge-cases
involving e.g. expiry may be exercised.
commit double-signing violations
[...] (this list is non-exhaustive at the time of writing)

this will allow us to run integration tests faster, write them more easily,
and generally improve our ability to make assurances about the correctness
of our software.

📐 requirements & design guidelines

(this will go into crate-level documentation at some point)

the mock consensus engine should be application agnostic. that means it should not depend on penumbra-* crates. (#3810)
the mock consensus engine is built to drive a consensus service C: Service<ConsensusRequest, Response = ConsensusResponse, Error = BoxError>

🔗 related work

🔜 future work

this is a large project, and the extension and maintenance of this library will be an ongoing project. here are things that we ought to do someday, but won't be considered requirements for this issue to be closed.

❌ out-of-scope

things we will not be doing, and other closed tickets:

👓 further reading

The text was updated successfully, but these errors were encountered:

cratelyn · 2024-01-09T23:24:02Z

update 2024-01-09: in #3592 and in some direct conversations, we discussed the desire to address #3588 using two crates: (1) a core library that provides the ability to generate block headers, and (2) an in-memory consensus engine with facilities to manipulate time.

after some further reading, i believe it makes sense to keep #3597 focused on (1), rather than attempting to completely address #3588 in one PR. this core library should be able to generate valid block headers that can be successfully parsed by a real light client library. (tendermint-light-client-verifier seems promising for this)

today i spent time reading through the broad strokes of how the protobuf types and .proto files are used to generate Rust types that can be (de)serialized and used by core application logic. i also spent some time today learning more about how the cnidarium crate's StateRead and StateWrite traits are extended by the penumbra-chain and penumbra-ibc libraries in crates/core/component. the latter part led me to some the message handling code, which interacts with cometbft/tendermint headers.

at this point i'm still getting a lay of the land, but i feel like i am gaining a much clearer picture of what work is to come. tomorrow i'll meet up with @avahowell to answer questions i've accrued concerning IBC, and spend some time working on stubs to generate a valid tendermint block header.

🍞 🔗 helpful links for future reference

https://github.com/penumbra-zone/ibc-types
https://rustdoc.penumbra.zone/main/ibc_types/lightclients/tendermint/client_state/struct.ClientState.html
https://crates.io/crates/tendermint-light-client-verifier
https://crates.io/crates/tendermint-light-client
Header structure:

penumbra/crates/proto/src/gen/tendermint.types.rs

Line 104 in 29b7cb2

pub struct Header {

cratelyn · 2024-01-22T17:04:01Z

update 2024-01-22:

today i continued to push pr #3597 along. i am at the point where i can almost feed output into the tendermint-light-client-verifier.

as of now, i have a lightweight validator system defined, defaulting to a single validator. i have my consensus state defined, modeled after (some of) comet's own internal consensus state.

there is still much more to do, but it's tremendously exciting to see this library progressing past the embryonic stages. h/t to conor for showing me some relevant plumbing in the pd daemon today as well.

that's all from me for this week, i will see you all on monday 💐

- me, #status channel in discord

with my draft pr (#3597) further along, i am going to pivot focus to work on #3627 this week. i'll resume work on this issue afterwards.

cratelyn · 2024-01-30T18:21:18Z

today @hdevalence linked me to #1664. this is a great issue outlining the larger goals and direction of our testing facilities. linking here, for future reference.

cratelyn · 2024-02-06T23:24:17Z

update 2024-02-06: my focus this week has been scoping and specifying the work to be done, and filing tickets. see ☝️ #3740, #3741, #3753, #3754, #3755, #3756, #3757, #3758, #3759, #3760.

i'll be meeting with Henry tomorrow to talk more about this, and hopefully pair program a bit.

hdevalence · 2024-02-06T23:50:04Z

I think it would be good to focus this effort on the concrete goal of writing App-wide tests, which is the purpose of building the mock engine. We already have two of these, which were added in the original testing push in #1664 / #1675:

app/src/tests/spend.rs
app/src/tests/swap_and_swap_claim.rs

We should start by writing a second version of each of those tests, but instead of driving each component's code manually in the #[test] function, we'd just instruct the mock engine to do high-level steps. This will ensure we're not overbuilding the test infrastructure relative to what we need.

Let's look at the first, simpler test, in spend.rs. In what follows I'll make up some stub API along the way, but this is just to convey a general shape, I'm not opinionated about the specifics. I'll also take the liberty of slightly rearranging the blocks of code in the existing test to fit a narrative order.

#[tokio::test]
async fn spend_happy_path() -> anyhow::Result<()> {
    let mut rng = rand_chacha::ChaChaRng::seed_from_u64(1312);

    let storage = TempStorage::new().await?.apply_default_genesis().await?;
    let mut state = Arc::new(StateDelta::new(storage.latest_snapshot()));

    let height = 1;

This is initializing a new TempStorage with a default genesis data. Instead, this should look something like this:

#[tokio::test]
async fn spend_happy_path_2() -> anyhow::Result<()> {
    let mut rng = rand_chacha::ChaChaRng::seed_from_u64(1312);

    let storage = TempStorage::new().await?;
    let app = App::new(storage.latest_snapshot());
    let engine = MockComet::builder()
        .single_validator() // builder-style methods for configuring validators
        .app_state(genesis::AppState::default()) // N.B. will later need an ext trait
        .init_chain(app) // "finish" method of builder, saves app and calls InitChain
        .await?;

As before, we create a TempStorage for an RAII-guarded temporary storage that only lives as long as the test. However, instead of manually writing in the genesis state, we first construct a complete App instance. Then, we instantiate a MockComet and use a builder API to configure it, before passing in the app to call init_chain.

The note about the extension trait is that we'll later want to have an extension trait for the builder API that does Penumbra-specific behavior, because the Penumbra application needs to know about its validators at genesis, so we'll eventually want to have a method that can "reconcile" a default genesis state with whatever validators the MockComet was configured with. But I think this is not particularly important to start off with, until we seek to exercise the staking component.

Let's keep going.

    // ORIGINAL spend_happy_path()

    // Precondition: This test uses the default genesis which has existing notes for the test keys.
    let mut client = MockClient::new(test_keys::FULL_VIEWING_KEY.clone());
    let sk = test_keys::SPEND_KEY.clone();
    client.sync_to(0, state.deref()).await?;
    let note = client.notes.values().next().unwrap().clone();
    let note_commitment = note.commit();
    let proof = client.sct.witness(note_commitment).unwrap();
    let root = client.sct.root();
    let tct_position = proof.position();

This section uses the existing MockClient to mock a Penumbra client syncing the chain. Remember that Penumbra distinguishes between public data, kept on-chain, and user data, kept off-chain on the user device. So to perform actions, we not only need a way to mock the consensus engine, but also a way to mock a client.

The MockClient is a minimal reference implementation of the Penumbra client logic (as compared to the entire Rust view server, which has a ton of incidental complexity), accessing the state directly rather than making network calls and only keeping data in memory.

This code would remain unchanged. If the complexity of the actions we wanted to perform in tests grew, we could either hook up the complete view server implementation, or add adapters to the MockClient. In any case, it's not the problem we need to address right now.

    // ORIGINAL spend_happy_path()
    // 1. Simulate BeginBlock
    let mut state_tx = state.try_begin_transaction().unwrap();
    state_tx.put_block_height(height);
    state_tx.put_epoch_by_height(
        height,
        Epoch {
            index: 0,
            start_height: 0,
        },
    );
    state_tx.apply();

    // 2. Create a Spend action
    let spend_plan = SpendPlan::new(&mut rng, note, tct_position);
    let dummy_effect_hash = [0u8; 64];
    let rsk = sk.spend_auth_key().randomize(&spend_plan.randomizer);
    let auth_sig = rsk.sign(&mut rng, dummy_effect_hash.as_ref());
    let spend = spend_plan.spend(&test_keys::FULL_VIEWING_KEY, auth_sig, proof, root);
    let transaction_context = TransactionContext {
        anchor: root,
        effect_hash: EffectHash(dummy_effect_hash),
    };

    // 3. Simulate execution of the Spend action
    spend.check_stateless(transaction_context).await?;
    spend.check_stateful(state.clone()).await?;
    let mut state_tx = state.try_begin_transaction().unwrap();
    state_tx.put_mock_source(1u8);
    spend.execute(&mut state_tx).await?;
    state_tx.apply();

    // 4. Execute EndBlock
    let height = 1;
    let end_block = abci::request::EndBlock {
        height: height.try_into().unwrap(),
    };
    ShieldedPool::end_block(&mut state, &end_block).await;

    let mut state_tx = state.try_begin_transaction().unwrap();
    // ... and for the App, call `finish_block` to correctly write out the SCT with the data we'll use next.
    state_tx.finish_block(false).await.unwrap();

    state_tx.apply();

    Ok(())

In the rest of the test, the current code:

manually does some ad-hoc BeginBlock simulation (not exercising the real codepaths, just manually poking at the state)
creates a "detached" Spend action isolated from any transaction (this will never occur in reality) and does ad-hoc mocking to patch up the fact that there is no containing transaction
executes only that Spend action, isolated from any containing transaction
manually simulates EndBlock (but only for the ShieldedPool component, leaving the app in an invalid state).

Instead, this should use MockComet to execute the entire app logic, so we are testing the code paths we care about and creating complete state transitions:

    // NEW spend_happy_path_2()
    
    // 1. Create a `TransactionPlan` with a 1-spend, 1-output transaction
    let plan = TransactionPlan {
        // self-contained struct literal with a 1-spend, 1-output transaction
        // using the data obtained from the MockClient as in the snippet above
    };
    let auth_data = AuthorizationData {
        // we can either create a struct literal with the signatures we need,
        // or we can initialize a SoftKms instance with the test keys and pass the plan
    };
    let witness_data = WitnessData {
        // self-contained struct literal using the inclusion proof from the MockClient
    };
    
    // 2. Use the `TransactionPlan`, `AuthorizationData`, and `WitnessData` to build the transaction
    let tx = plan.build_concurrent(...).await?;
    
    // 3. Now use the `MockComet` to create a new block with that transaction
    engine.block_builder()
        //.timestamp(...) // methods for controlling simulated execution
        .add_tx(tx.encode_to_vec()) // adds to list in block builder
        .execute() // triggers app execution
        .await?;

This is much simpler and less error-prone. We need to change from simulating a single Spend action in isolation to simulating it in a complete transaction. But this is fine, because that's the actual code path we are going to be executing.

We manually plan the transaction using the data from the MockClient, then build it. Later, if the complexity of transaction planning grows, we may want to consider adapting the Rust view server into the test framework, but this is not important to worry about now.

Then, we use a block_builder() builder API on the MockComet instance to create a new block and execute it against the app. I'm imagining that in general, invocations of the builder API should look like:

zero or more calls to various methods for controlling simulated block production, like setting a timestamp, setting certain validators as not signing, and so on. The important thing here is that these all must be optional, with reasonable defaults, so that it's low-friction to write tests; for instance, if there was no call to .timestamp, there should be default of 5s relative offset, etc.
zero or more calls to add_tx, appending transactions into the block;
.execute(), which builds the block as-configured and then drives execution of the application.

Now, in this example, we can go even further with spend_happy_path_2():

    // NEW spend_happy_path_2()
    
    // 4. Use the MockClient to check that we detect our output note
    client.sync_to(...)
    // .... check that the note we expect is now seen in the MockClient
    
    // ... or we could make a second transaction, checking we can spend notes that weren't created at genesis
    // ... or we could check that a double-spend fails, or, ....

By contrast, what would happen if we tried to add this to the existing test? The entire test would explode, because the current test leaves the application in an invalid state, it doesn't write out the compact block, it doesn't close the SCT, etc., because it doesn't execute the full application logic.

Note also what's not required: anything other than actually driving the application and feeding it a header as part of BeginBlock. At this point, the header doesn't even need to be valid -- that would only come into play once we try to write an IBC test (#3758).

cratelyn · 2024-02-07T18:30:59Z

I think it would be good to focus this effort on the concrete goal of writing App-wide tests, which is the purpose of building the mock engine.

first: i've renamed this issue, to be clearer about the fact that this is not intended for intgegration tests, and is intended for App tests.

fixes #3933. this introduces a `fast_forward` method to a test node, allowing tests using the mock consensus engine (#3588) to fast forward a certain number of blocks. the existing `mock_consensus_can_send_a_sequence_of_empty_blocks` test is rewritten to make use of this method. this is done in service of #3995. --- * #3588 * #3933 * #3995

see #3913, #3973 and #3588. this is a second attempt, following up on #3980. #### 🔭 background NB: the difference between this and #3679 is that the latter (_which ran afoul of a regression_) would have `penumbra-app` create a `Routes`, that we would [add](https://github.com/penumbra-zone/penumbra/pull/3679/files#diff-fbc4204ceb976c8cb30ed06168e2476700bae21bfd803e26281b2d026194d430R204) to the builder (_which stays in `pd`_). here, i'm not trying to make that cut between `Router` and `Routes`, and am attempting to hoist the whole thing out of `pd`, without making changes to how we interact with `tonic`. my aim is for us to be able to move this, without running into that bug (#3697) again. NB: after running into problems in #3980, i found a way to easily reproduce the issue locally. my belief was that something related to our dependencies' cargo features was at play. rather than isolate the issue, it was easier to rewrite this (_it's just code motion, after all_) while running some of the network integration tests in a loop. unlike #3980, this moves the rpc server into `penumbra-app`, per #3980 (comment) #### 👁️ overview we would like to use the rust view server in mock consensus tests. in order to run the `penumbra_view::ViewServer` however, we need to spin up the corresponding grpc endpoint for it to connect to. this branch performs a bit of code motion, moving the `grpc_server` out of `pd` and into `penumbra-app`. there will likely be other functional changes to the code in question before we can use it in those tests, but this PR is interested in moving that code into a place where our tests can rely upon it.

fixes #3933. this introduces a `fast_forward` method to a test node, allowing tests using the mock consensus engine (#3588) to fast forward a certain number of blocks. the existing `mock_consensus_can_send_a_sequence_of_empty_blocks` test is rewritten to make use of this method. this is done in service of #3995. --- * #3588 * #3933 * #3995

) fixes #3936. based upon #4002. see #3588. this changes the type of the block builder's `data`, allowing it to be implicitly kept empty. this is useful when e.g. fast forwarding a number of blocks _(see #4002)_. this spares us the need to call `with_data(vec![])`, and additionally means that the block builder is never in an uninitialized state _(making #4003 needless)._ * #3936 * #4002 * #3588

this patches the mock consensus `TestNode::end_block` method so that the height of these requests does not stay at 1. this is needed for staking tests, see #3995. * #3588 * #4001 * #3995 * #3840

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 #3966)_ * adds a new `mock_consensus_can_define_and_delegate_to_a_validator` test that defines a post-genesis validator. _(see #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`. --- #### :link: related * #4009 * #4010 * #4011 * #4017 * #4027 * #4028 * #4029 * #3966 * #3908 * #3588 --------- Co-authored-by: Henry de Valence <hdevalence@penumbralabs.xyz>

fixes #3937. * #3937 * #3588 this adds a `two_validators` method to the test node builder, so that tests may set up a test node that has two validator keys.

fixes #4050. see also #3588. this introduces a test case that demonstrates that transactions' validator definition actions must have a valid authentication signature, or the validator will not be added to the set of known validators.

fixes #4181. see #3588. this makes a pass through the `penumbra-mock-consensus` library and further documents various interfaces. one other small tweak, logging a warning if a caller may be inadvertently discarding transactions, is made while we are here. these docs may be rendered by running: `cargo doc --package penumbra-mock-consensus --open` --------- Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

fixes #4050. see also #3588. this introduces a test case that demonstrates that transactions' validator definition actions must have a valid authentication signature, or the validator will not be added to the set of known validators. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > this is a test case.

see #3588. follows #4184 and #4181. this takes a pass through the shared, Penumbra-specific test infrastructure for mock consensus tests. notably, this decomposes `init_chain.rs`, which has now become somewhat redundant with the existence of other more involved tests of e.g. validator uptime tracking. this also cleans up some unused imports, guards against future occurrences of that issue (_sharing code in `tests/` files is awkward_), and decomposes the `common/mod.rs` file into some distinct standalone components. this also belatedly removes the `common::start_test_node()` helper. at some point (_i was unable to find the link_) it was suggested that we refrain from a shared setup helper like that. this branch removes that helper, and updates its call-sites. this branch is largely code motion, and is intended to be a last bit of cleanup as we prepare for #3588 to wind down. ❤️ --------- Co-authored-by: Henry de Valence <hdevalence@penumbralabs.xyz>

cratelyn added C-enhancement Category: an enhancement to the codebase A-tooling Area: developer tooling for building Penumbra itself labels Jan 8, 2024

cratelyn self-assigned this Jan 8, 2024

erwanor changed the title ~~🔬 mock consensus engine for integration tests~~ Tracking issue: mock consensus engine for integration tests Jan 8, 2024

erwanor changed the title ~~Tracking issue: mock consensus engine for integration tests~~ Release tracking issue: mock consensus engine for integration tests Jan 8, 2024

conorsch changed the title ~~Release tracking issue: mock consensus engine for integration tests~~ Tracking issue: mock consensus engine for integration tests Jan 8, 2024

This was referenced Jan 8, 2024

feat: 📝 implement cometstub crate (work-in-progress) #3592

Closed

feat: 📝 implement cometstub crate (work-in-progress) #3597

Closed

erwanor mentioned this issue Jan 15, 2024

stake: ignore byzantine misbehavior from unknown validators #3613

Merged

cratelyn mentioned this issue Jan 18, 2024

Clean up pd auto-https code #3627

Closed

This was referenced Jan 26, 2024

pd: 👏 cli commands and options live in pd::cli #3672

Merged

pd: 🐣 move entrypoint logic into app library #3695

Closed

cratelyn changed the title ~~Tracking issue: mock consensus engine for integration tests~~ Tracking issue: 🤝 mock consensus engine for integration tests Jan 31, 2024

cratelyn changed the title ~~Tracking issue: 🤝 mock consensus engine for integration tests~~ Tracking issue: 🤝 mock consensus engine for App tests Feb 7, 2024

This was referenced Mar 12, 2024

mock-consensus: 🔀 test node can fast_forward #4002

Merged

mock-consensus: 👷 block data is implicitly empty, can be appended #4004

Merged

cratelyn mentioned this issue Mar 15, 2024

fix(mock-consensus): 🍰 fix two height bugs in mock consensus tests #4028

Merged

This was referenced Mar 18, 2024

tests: 🏃 mock consensus can exercise staking component #4001

Merged

tests: 🖋️ mock consensus engine can test validator uptime measurements #4040

Closed

This was referenced Mar 19, 2024

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

Merged

tests(app): ⛓ use mock consensus for IBC tests #4071

Open

Support threshold custody for validators #4037

Merged

cratelyn added a commit that referenced this issue Apr 8, 2024

mock-consensus: 🫛 two_validators adds two keys

70b8c8e

fixes #3937. * #3937 * #3588 this adds a `two_validators` method to the test node builder, so that tests may set up a test node that has two validator keys.

cratelyn added a commit that referenced this issue Apr 8, 2024

mock-consensus: 🫛 two_validators adds two keys

7a7d51c

fixes #3937. * #3937 * #3588 this adds a `two_validators` method to the test node builder, so that tests may set up a test node that has two validator keys.

cratelyn mentioned this issue Apr 8, 2024

mock-consensus: 🫛 two_validators adds two keys #4174

Merged

1 task

cratelyn added a commit that referenced this issue Apr 9, 2024

mock-consensus: 🫛 two_validators adds two keys

56c67ca

fixes #3937. * #3937 * #3588 this adds a `two_validators` method to the test node builder, so that tests may set up a test node that has two validator keys.

cratelyn mentioned this issue Apr 9, 2024

tests: 🧼 document and polish penumbra-mock-consensus library #4181

Closed

This was referenced Apr 9, 2024

tests(app): 🙅 app rejects invalid auth signatures #4182

Merged

docs(mock-consensus): 🦅 document various testing interfaces #4184

Merged

cratelyn mentioned this issue Apr 9, 2024

tests(app): 💎 polish mock consensus test infrastructure #4185

Merged

1 task

cratelyn closed this as completed Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cratelyn commented Jan 8, 2024 •

edited

Loading

cratelyn commented Jan 9, 2024

cratelyn commented Jan 22, 2024 •

edited

Loading

cratelyn commented Jan 30, 2024

cratelyn commented Feb 6, 2024

hdevalence commented Feb 6, 2024

cratelyn commented Feb 7, 2024

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

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

Comments

cratelyn commented Jan 8, 2024 • edited Loading

💭 background and motivation

📐 overview

📐 requirements & design guidelines

🔗 related work

🔜 future work

❌ out-of-scope

👓 further reading

cratelyn commented Jan 9, 2024

🍞 🔗 helpful links for future reference

cratelyn commented Jan 22, 2024 • edited Loading

cratelyn commented Jan 30, 2024

cratelyn commented Feb 6, 2024

hdevalence commented Feb 6, 2024

cratelyn commented Feb 7, 2024

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

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

cratelyn commented Jan 8, 2024 •

edited

Loading

cratelyn commented Jan 22, 2024 •

edited

Loading