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

Fee unshielding #1281

Closed
wants to merge 57 commits into from
Closed

Fee unshielding #1281

wants to merge 57 commits into from

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Apr 11, 2023 •
edited
Loading

Based on #1242.

Addresses #1010.

Adds an optional, unencrypted unshielding Transaction to the WrapperTx to allow unshielding some tokens to pay fees when the transparent balance is insufficient.

  • Static and dynamic (execution simulation) checks of the unshielding in mempool_validate, process_proposal and prepare_proposal
  • Performs the unshield in finalize_block when the wrapper is accepted
  • Updates the client to allow the user to pass a spending key to produce the unshield
  • Adds an e2e test

@grarco grarco force-pushed the grarco/fee-unshielding branch 2 times, most recently from d11b061 to 6a857ca Compare April 13, 2023 15:05
@grarco
Copy link
Contributor Author

grarco commented Apr 13, 2023

pls update wasm

grarco added a commit that referenced this pull request Apr 13, 2023
@grarco
changelog: add #1281
fe42313
grarco added a commit that referenced this pull request Apr 14, 2023
@grarco
changelog: add #1281
ff23f8d
grarco added a commit that referenced this pull request Apr 15, 2023
@grarco
changelog: add #1281
f86e7e1
grarco added a commit that referenced this pull request Apr 17, 2023
@grarco
changelog: add #1281
6f89417
@grarco
Copy link
Contributor Author

grarco commented Apr 17, 2023

pls update wasm

grarco added a commit that referenced this pull request Apr 17, 2023
@grarco
changelog: add #1281
abdcbe7
@grarco grarco mentioned this pull request Apr 17, 2023
Closed
4 tasks
grarco
grarco commented Apr 17, 2023
View reviewed changes
apps/src/lib/node/ledger/shell/finalize_block.rs Outdated
// from this function but try to take the funds from the unshielded
// balance
match apply_tx(
TxType::Decrypted(DecryptedTx::Decrypted {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In here, I fake a decrypted tx to execute the unshielding. It's basically the same trick we use to execute governance proposals. At this point, I don't know whether we should think about allowing Raw txs execution (making sure that no other Raw txs can end up in a block and be executed)

@grarco grarco marked this pull request as ready for review April 17, 2023 15:01
@grarco grarco requested a review from tzemanovic April 17, 2023 15:01
grarco
grarco commented Apr 18, 2023
View reviewed changes
apps/src/lib/node/ledger/shell/prepare_proposal.rs
}
if let Ok(TxType::Wrapper(_)) = process_tx(tx) {
return Some(tx_bytes.clone());
match self.validate_wrapper_bytes(tx_bytes, &mut temp_block_gas_meter, block_time, &mut temp_wl_storage, &gas_table, &mut vp_wasm_cache, &mut tx_wasm_cache) {
Copy link
Contributor Author

@grarco grarco Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gas check inside validate_wrapper_bytes could actually be extracted from there and moved into the BlockSpaceAllocator, effectively transforming it into a BlockSpaceGasAllocator. It would probably make for a more coherent logic but would not improve performance. Actually, given that the allocator stops at the first element of the iterator that cannot be placed in the block, we could end up with blocks containing less transactions. Still, I like the idea of a SpaceGas allocator better

tzemanovic
tzemanovic reviewed Apr 28, 2023
View reviewed changes
apps/src/lib/client/signing.rs
@@ -146,31 +146,31 @@ pub async fn tx_signer(
///
/// If it is a dry run, it is not put in a wrapper, but returned as is.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// If it is a dry run, it is not put in a wrapper, but returned as is.
/// If it is a dry run, it is not put in a wrapper, but returned as is.
///
/// If the tx fee is to be unshielded, it also returns the unshielding epoch.

tzemanovic
tzemanovic reviewed May 2, 2023
View reviewed changes
core/src/types/transaction/wrapper.rs Outdated
@@ -171,6 +173,8 @@ pub mod wrapper_tx {
pub epoch: Epoch,
/// Max amount of gas that can be used when executing the inner tx
pub gas_limit: GasLimit,
/// The optional, unencrypted, unshielding tx for fee payment
pub unshield: Option<Tx>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rather than using the Tx type here which allows arbitrary data and code, which would require some validation in the protocol (and also bloats the payload), it would be better to only attach masp Transaction here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be easy to implement given that we've already added the support for tx hashes in #1297. We can load the wasm code before running the operation. Still, some validation will be necessary even though we can get rid of the check on the content of the unshielding tx that will be done now with the type system

@adrianbrink adrianbrink mentioned this pull request May 12, 2023
Closed
10 tasks
grarco added a commit that referenced this pull request May 21, 2023
@grarco
changelog: add #1281
3847cba
@grarco
Copy link
Contributor Author

grarco commented May 21, 2023

pls update wasm

1 similar comment
@grarco
Copy link
Contributor Author

grarco commented May 22, 2023

pls update wasm

@grarco
Copy link
Contributor Author

grarco commented May 22, 2023

pls update wasm

@grarco grarco force-pushed the grarco/fee-unshielding branch 2 times, most recently from 6b51c46 to 9870331 Compare May 23, 2023 11:45
grarco added a commit that referenced this pull request May 23, 2023
@grarco
changelog: add #1281
5f28abe
grarco and others added 25 commits June 5, 2023 10:18
@grarco
Tests fee unshielding
a309c52
@grarco
Fixes args and signer of unshielding fee tx
4046007
@grarco
Moves write log commit out of wrapper_fee_check
d1b31a5
@grarco
Provides a clean write log for unshielding tx in process_proposal
776b1ad
@grarco
Fixes unit tests
d1f50a2
@grarco
Refactors fee unshielding construction
7b62ff3
@grarco
Clippy + fmt
61ae66f
@grarco
Refactors unshield field to Transaction
c8cd140
@grarco
Fixes fee unshielding amount computation
2267fa0
@grarco
Fixes transfer wasm code load from storage
458ae4b
@grarco
Allows client to submit tx without required fee unshielding
0bbe263
@grarco
Fixes e2e fee unshielding test
47abacd
@grarco
Adds a fee unshielding gas limit protocol parameter
9ce3f0a
@grarco
Adds a fee unshielding descriptions limit protocol parameter
2eacc8d
@grarco
Refactors \'generate_fee_unshielding\'
5443962
@grarco
Refactors return type of \'wrapper_fee_check\'
0b438cb
@grarco
Refactors fee errors
8c001d7
@grarco
Updates unit tests error messages
ce247cd
@grarco
Removes unused error variants
a127f29
@grarco
Adds missing protocol parameters to e2e genesis file
e8e8d9a
@grarco
Adds fee unshielding descriptions check in client
337e7e4
@grarco
Fixes wrong logic in fee payment
ef9af90
@grarco
Clippy + fmt
5ac6252
@github-actions @grarco
[ci] wasm checksums update
2ec9bb6
@grarco
changelog: add #1281
b9f7485
@grarco grarco requested a review from murisi June 20, 2023 10:24
@grarco grarco mentioned this pull request Jul 5, 2023
Merged
2 tasks
@grarco
Copy link
Contributor Author

grarco commented Aug 10, 2023

Closed in favor of #1327

@grarco grarco closed this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzemanovic tzemanovic tzemanovic left review comments

@cwgoes cwgoes cwgoes left review comments

@murisi murisi Awaiting requested review from murisi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grarco @tzemanovic @cwgoes