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.

Sign up for GitHub

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

Jump to bottom

mock-consensus: 💰 execute a spend via mock consensus #3857

Merged
merged 1 commit into from
Feb 22, 2024
Merged

mock-consensus: 💰 execute a spend via mock consensus #3857

merged 1 commit into from
Feb 22, 2024

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Feb 20, 2024 •
edited
Loading

f/ixes #3788.

this introduces a new mock_consensus_can_send_a_spend_action test which, as the name implies, shows that the mock consensus harness can send and execute a Spend action via our consensus service.

a MockClient::with_sync_to_storage helper (bikesheds are welcome here) is introduced, to facilitate syncing to the latest snapshot of the temporary storage.

@cratelyn
mock-consensus: 💰 execute a spend via mock consensus
988568d 
fixes #3788.

this introduces a new `mock_consensus_can_send_a_spend_action` test
which, as the name implies, shows that the mock consensus harness can
send and execute a `Spend` action via our consensus service.

a `MockClient::with_sync_to_storage` helper (bikesheds are welcome here)
is introduced, to facilitate syncing to the latest snapshot of the
temporary storage.

* #3588
* #3788
@cratelyn cratelyn added C-enhancement Category: an enhancement to the codebase A-testing Area: Relates to testing of Penumbra A-mock-consensus Area: Relates to the mock consensus engine _P-high High priority labels Feb 20, 2024
@cratelyn cratelyn added this to the Sprint 0 milestone Feb 20, 2024
@cratelyn cratelyn self-assigned this Feb 20, 2024
@cratelyn cratelyn mentioned this pull request Feb 20, 2024
Closed
@cratelyn cratelyn marked this pull request as ready for review February 22, 2024 15:30
@cratelyn
Copy link
Contributor Author

hdevalence [penumbra]
i think the planner API won't be immediately helpful here, since we're using the mock client, not a full view server. i think i described some bits of this in the comment i left describing how the rewritten tests would look.

the only thing that seems suspicious is the chain id. otherwise: expiry height 0 means no expiry, and default fee is 0, which checks out?

for posterity: feedback from #status in discord.

we should use TransactionPlan. i'm going to handle that in a follow-on, and have altered the description of the pr so that it won't close out #3788. i've also opened #3864 as a little tweak, along the way.

@cratelyn cratelyn merged commit 1381ca8 into main Feb 22, 2024
6 checks passed
@cratelyn cratelyn deleted the kate/deliver-a-transaction-to-mock-consensus branch February 22, 2024 15:36
@cratelyn cratelyn mentioned this pull request Feb 22, 2024
Closed
cratelyn added a commit that referenced this pull request Feb 22, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan to add spends
56ca685 
* #3588.
* #3788.
* #3857
@cratelyn cratelyn mentioned this pull request Feb 22, 2024
Merged
cratelyn added a commit that referenced this pull request Feb 23, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan to add spends
1171d94 
* #3588.
* #3788.
* #3857

---

TODO(kate): currently, i'm seeing an error shaped like so:

```
2024-02-23T02:55:09.755882Z  INFO penumbra_app::server::consensus: deliver_tx failed, e: binding signature failed to verify

Caused by:
    Invalid signature.
    at crates/core/app/src/server/consensus.rs:210
    in penumbra_mock_consensus::abci::deliver_tx
    in penumbra_mock_consensus::block::execute with height: 1, time: 1708656909
```
cratelyn added a commit that referenced this pull request Feb 26, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan to add spends
81eea77 
* #3588.
* #3788.
* #3857

---

TODO(kate): currently, i'm seeing an error shaped like so:

```
2024-02-23T02:55:09.755882Z  INFO penumbra_app::server::consensus: deliver_tx failed, e: binding signature failed to verify

Caused by:
    Invalid signature.
    at crates/core/app/src/server/consensus.rs:210
    in penumbra_mock_consensus::abci::deliver_tx
    in penumbra_mock_consensus::block::execute with height: 1, time: 1708656909
```
cratelyn added a commit that referenced this pull request Feb 26, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan in tests
f553338 
* #3588
* #3788
* #3857

this rewrites `mock_consensus_can_send_a_spend_action` so that it uses
the `TransactionPlan` API.
cratelyn added a commit that referenced this pull request Feb 26, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan in tests
5cf7162 
* #3588
* #3788
* #3857

this rewrites `mock_consensus_can_send_a_spend_action` so that it uses
the `TransactionPlan` API.
cratelyn added a commit that referenced this pull request Feb 26, 2024
@cratelyn
mock-consensus: ⭐ use transaction plan in tests (#3866)
ad7d109 
* #3588
* #3788
* #3857
* #3880
* #3881
@cratelyn cratelyn mentioned this pull request Feb 26, 2024
Merged
cratelyn added a commit that referenced this pull request Feb 27, 2024
@cratelyn @hdevalence
app: 💸 a mock consensus spend is performed (#3891)
e241ef4 
fixes #3788.

this branch builds upon the work in #3857 and #3866, filling out the
remaining holes in the transaction plan. this now performs a spend,
using the mock consensus engine.

---------

Co-authored-by: Henry de Valence <hdevalence@penumbralabs.xyz>
@cratelyn cratelyn mentioned this pull request May 29, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cratelyn cratelyn

Labels
A-mock-consensus Area: Relates to the mock consensus engine A-testing Area: Relates to testing of Penumbra C-enhancement Category: an enhancement to the codebase _P-high High priority
Projects
Penumbra
Status: Done
Milestone
Sprint 0
Development

Successfully merging this pull request may close these issues.
1 participant
@cratelyn