Skip to content

Conversation

@TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Apr 1, 2024

References #4081 to simplify and fix transaction planner logic in the context of non-zero fees. Additionally expands scope to consume #4155.

Blocked until fee restructuring #3940 is merged.

#4154 and #3435 will follow.

@TalDerei TalDerei marked this pull request as ready for review April 2, 2024 20:26
@TalDerei
Copy link
Collaborator Author

TalDerei commented Apr 4, 2024

@hdevalence @zbuc the planner logic has been refactored to account for the presence of non-zero gas fees, following the gas schedule changes that added block space costs for every action in #3940. There's a few things to note here.

  • 1. we introduced prioritize_and_filter_spendable_notes, enabling a custom note prioritization scheme based on (1) one-time addresses, or (2) largest value. In the case of (2), the order of the notes being consumed seemed to matter in the original planner logic. For instance, for a specific user balance and non-zero gas fee, consuming smaller note N1 and larger note N1 (where N0 + N1 > Fee) failed, whereas consuming larger note N1 and smaller note N0 (where N1 + N0 > Fee) succeeded. Currently adding a few non-zero gas fee tests that check this commutativity property.

  • 2. we need to double check the correctness of the modified planner to be absolutely sure we're not double counting fees. For instance, In the case where we can't sufficiently balance the transaction in the first while let loop iteration, we continue iterating and adding additional spend notes. In that case, we're calling adjust_change_for_fee multiple times, subtracting fees from the change notes. The fee is a function of the actions / output change in the planner. If the first iteration has output (amount we want to spend), spend (note contributing to that spend), and output (change note)...and we iterate again to add another spend note, we now have output, spend, spend, output. Does that mean we're double counting the fees being subtracted from the change note? Further investigation is required to ensure avoiding making transactions unnecessarily more expensive than they need to be.

  • 3. the delegate voting functionality isn't being properly balanced, causing smoke test failures in pcli. Additionally, how should IBC actions be handled? Another set of eyes here would be really helpful.

  • 4. brainstorm other edge cases

@conorsch after we can proceed with #4154 and check how non-zero gas fees fare in our smoke / integration tests on devnet. This requires modifying smoke-test.sh.

@cratelyn I know this fee work is still blocking #3973, but I think we should be overly cautious with this and spend some more time sanity checking.

@cratelyn cratelyn removed their request for review April 24, 2024 23:18
@cratelyn
Copy link
Contributor

removing my review request for now, while the smoke tests get sorted out, but ping me when this is ready for a look!

@TalDerei TalDerei added the A-fees Area: Relates to transaction fees label May 1, 2024
@TalDerei
Copy link
Collaborator Author

TalDerei commented May 2, 2024

a message to all reviewers: this has ballooned in scope into what now resembles frankensteins monster, falling out of parity with the web extension. In favor of maintaining that parity and reducing scope / making it easier to review, the port will be consumed by #4300.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-fees Area: Relates to transaction fees C-design Category: work on the design of Penumbra C-refactor Category: refactors and other related improvements _P-V1 Priority: slated for V1 release

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants