Fix masp events by grarco · Pull Request #4449 · namada-net/namada

grarco · 2025-03-05T16:23:04Z

Describe your changes

Partially addresses #3824 (removes ExtendedTxResult).
Partially addresses #3827 as we take events out of BatchedTxResult in case of masp fee payment.

Ensures that events for masp fee payment txs are emitted as soon as the storage changes are committed

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes
If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
- Relevant PR if applies:
If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
- Relevant PR if applies:
  - ~~Update masp events namada-masp-indexer#50~~ ->Update masp events adjusted namada-masp-indexer#64
  - Update masp events namada-indexer#331

codecov · 2025-03-05T16:50:56Z

Codecov Report

Attention: Patch coverage is 82.46206% with 104 lines in your changes missing coverage. Please review.

Project coverage is 74.51%. Comparing base (f3cc9da) to head (4d9efbd).
Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sdk/src/masp/utilities.rs	35.84%	34 Missing ⚠️
crates/sdk/src/masp.rs	56.92%	28 Missing ⚠️
crates/node/src/dry_run_tx.rs	39.02%	25 Missing ⚠️
...tes/shielded_token/src/masp/shielded_sync/utils.rs	89.76%	13 Missing ⚠️
crates/tx/src/event.rs	92.85%	2 Missing ⚠️
crates/node/src/shell/finalize_block.rs	97.50%	1 Missing ⚠️
crates/shielded_token/src/masp/test_utils.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4449      +/-   ##
==========================================
+ Coverage   74.48%   74.51%   +0.03%     
==========================================
  Files         339      339              
  Lines      110643   110865     +222     
==========================================
+ Hits        82412    82612     +200     
- Misses      28231    28253      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

batconjurer · 2025-03-06T21:54:51Z

crates/node/src/protocol.rs

    CA: 'static + WasmCacheAccess + Sync,
 {
-    for cmt in get_batch_txs_to_execute(tx, &extended_tx_result.masp_tx_refs) {
+    let ran_masp_fee_payment = !tx_result.is_empty();


this line seems bug prone. Took me forever to confirm this works

I think we can check if a masp event is in the values, no?

We could but there's also another issue #3827 with duplicated events. If we were to ever fix that I believe we won't have those events in here anymore (unless we also implement your suggestion to postpone pushing the events) and we could fail here.

Should we just pass a flag to this function to signal that the first tx already ran to pay fees?

Got your point, will work on a fix to avoid relying on the tx result

There's a tentative fix in a0b94a5

batconjurer · 2025-03-06T22:04:59Z

crates/node/src/shell/finalize_block.rs

+                    // Emit the events of the fee-paying inner tx. This tx has
+                    // just been committed so we need to ensure the relative
+                    // events are emitted
+                    let events = tx_result.0.first_key_value().map_or_else(


Again, I find this assumption dangerous. Maybe we can soup up the type to keep thses events under a separate field

We can but we need to handle the case of a failing atomic batch for which the fee paying tx should be committed anyway (and the relative events should be emitted). My reason to emit the events here is that in the line above we commit the storage changes and I wanted to emit the events at the same time.

Also, by emitting the events here, the masp events are already sorted in the block results (without the need to look at the masp fee payment attribute), so that clients don't need to reorder them.

crates/sdk/src/masp.rs

batconjurer · 2025-03-06T22:16:16Z

crates/sdk/src/masp/utilities.rs

+            for MaspEvent {
                tx_index,
-                masp_refs,
+                kind: _,


I cant tell where this field is used

It is used in the event itself (which could be of type masp/transfer or masp/fee-payment), and after 837d480 it is also used when fetching txs from the indexer since when we cache them we need to be able to sort them correctly (as suggested by @sug0) and we need the tx kind for that since fee payment txs need to be move ahead of the queue

crates/sdk/src/masp.rs

crates/node/src/protocol.rs

sug0

couple of suggestions. let's not merge this until we thoroughly test integration with the associated masp indexer pr.

sug0 · 2025-03-08T08:00:23Z

crates/node/src/protocol.rs

+                        // Stop the execution of an atomic batch at the
+                        // first failed transaction
+                        return Err(DispatchError {
+                            error: Error::FailingAtomicBatch(cmt.get_hash()),
+                            tx_result: Some(tx_result),


do we drop the batch later?

Yes we do that in handle_batch_error (exception made for the masp fee paying tx that would be committed even in case of a failing atomic batch)

sug0 · 2025-03-08T08:04:15Z

crates/node/src/shell/finalize_block.rs

+                    // Emit the events of all the inner txs that have been
+                    // successfully executed when handling the wrapper. These


does this imply that we do not emit the events of failing txs? hm. I was thinking what would happen with IBC ack failures, but I guess those are still successful tx executions (despite of the failure semantics).

That's correct, I asked this question to @yito88 a few days ago, it seemed ok to me but maybe we can discuss it more in depth

crates/sdk/src/masp.rs

sug0 · 2025-03-10T10:32:02Z

crates/sdk/src/masp.rs

    // NOTE: It is possible to have two identical references in a same batch:
    // this is because, some types of MASP data packet can be correctly executed
    // more than once (output descriptions). We have to make sure we account for
    // this by using collections that allow for duplicates (both in the args
    // and in the returned type): if the same reference shows up multiple
    // times in the input we must process it the same number of times to
    // ensure we contruct the correct state


this comment doesn't make sense anymore, we're returning a single Transaction (rather than a Vec of them)

Done in 053eb19 👍🏻

sug0 · 2025-03-10T10:33:21Z

crates/sdk/src/masp.rs

+            // Check if the event is a Masp one
+            let Ok(kind) = namada_events::EventType::from_str(&event.kind)
+            else {
+                return Ok(None);
+            };
+
+            let kind = if kind == namada_tx::event::masp_types::TRANSFER {
+                MaspEventKind::Transfer
+            } else if kind == namada_tx::event::masp_types::FEE_PAYMENT {
+                MaspEventKind::FeePayment
+            } else {
+                return Ok(None);
+            };
+
+            // Extract the data from the event's attributes, propagate errors if
+            // the masp event does not follow the expected format
+            let data =
+                MaspTxRef::read_from_event_attributes(&event.attributes)?;
+            let tx_index =
+                IndexedTx::read_from_event_attributes(&event.attributes)?;
+
+            Ok(Some(MaspEvent {
+                tx_index,
+                kind,
+                data,
+            }))


impl a TryFrom<Event> for MaspEvent

sug0 · 2025-03-10T10:38:41Z

crates/shielded_token/src/masp/shielded_sync/utils.rs

+    /// The masp tx kind, fee-payment or transfer
+    pub kind: MaspEventKind,


I know it feels like code duplication, but it might be better to have a separate type for the kind of masp indexed tx. we might want to introduce other masp event kinds that have nothing to do with the way we intend to sort masp indexed txs.

Done in 4d9efbd

sug0 · 2025-03-10T10:39:00Z

crates/shielded_token/src/masp/shielded_sync/utils.rs

 use namada_tx::{IndexedTx, IndexedTxRange};
+use serde::{Deserialize, Serialize};
+
+/// An indexed masp tx carrying information on wether it was a fee paying tx or


Suggested change

/// An indexed masp tx carrying information on wether it was a fee paying tx or

/// An indexed masp tx carrying information on whether it was a fee paying tx or

sug0 · 2025-03-10T10:41:07Z

crates/shielded_token/src/masp/shielded_sync/utils.rs

+            .map(|(masp_indexed_tx, transaction)| {
+                (masp_indexed_tx.indexed_tx, transaction)
+            })
+            .collect::<BTreeMap<_, _>>()


Let's not collect a new BTreeMap here. The contains_height method is supposed to be relatively cheap. We can modify IndexedTxRange to refer to a MaspIndexedTxRange instead. Lmk if you need help with this.

I've created a new MaspIndexedTxRange type for this purpose in 7df026f. I've also left IndexedTxRange untouched as I feel it could come in handy in the future or to current sdk users

grarco · 2025-03-18T11:52:01Z

@tzemanovic, @sug0, @batconjurer, @murisi I'm closing this in favor of #4475 which is the rebased version of this branch of top of main where I've removed the changes brought in by #4442

grarco added 2 commits March 5, 2025 17:21

Fixes masp events

1a29881

Removes ExtendedTxResult and emits masp events directly

d375e93

github-actions bot added the breaking:api public API breaking change label Mar 5, 2025

Changelog #4449

6b4b205

grarco requested review from batconjurer, murisi, sug0 and tzemanovic March 5, 2025 17:34

This was referenced Mar 5, 2025

Reduce masp events #3824

Open

Update masp events namada-net/namada-masp-indexer#50

Closed

batconjurer requested changes Mar 6, 2025

View reviewed changes

Fetched masp txs contains data about fee payment

837d480

tzemanovic reviewed Mar 7, 2025

View reviewed changes

crates/sdk/src/masp.rs Outdated Show resolved Hide resolved

tzemanovic reviewed Mar 7, 2025

View reviewed changes

crates/sdk/src/masp.rs Outdated Show resolved Hide resolved

tzemanovic reviewed Mar 7, 2025

View reviewed changes

crates/node/src/protocol.rs Outdated Show resolved Hide resolved

grarco added 2 commits March 7, 2025 16:19

Improves handling of wrapper events

a0b94a5

Explicitly imports BlockHeight

2921798

brentstone added this to the Pre-phase 4 consensus breaking release milestone Mar 7, 2025

Returns error in sdk if masp event is malformed

fe6996c

grarco mentioned this pull request Mar 8, 2025

Update masp events namada-net/namada-indexer#331

Closed

Fallible event kind conversion from string

9503775

sug0 requested changes Mar 10, 2025

View reviewed changes

grarco added 6 commits March 11, 2025 12:19

Takes wrapper events to avoid emitting them more than once

20776e6

Fixes Ord implementation for MaspIndexedTx

6e31201

MaspIndexedTx instead of IndexedTx in shielded context

42f13c7

Removes outdated comment

053eb19

Adds range for MaspIndexedTx

7df026f

Detaches MaspEventKind from MaspIndexedTx

4d9efbd

grarco added the ledger label Mar 11, 2025

grarco mentioned this pull request Mar 12, 2025

Update masp events adjusted namada-net/namada-masp-indexer#64

Merged

grarco added a commit that referenced this pull request Mar 17, 2025

Changelog #4449

22df680

grarco mentioned this pull request Mar 18, 2025

Fix masp events [rebase] #4475

Merged

3 tasks

grarco closed this Mar 18, 2025

grarco added a commit that referenced this pull request Mar 20, 2025

Changelog #4449

8ae9386

		// Emit the events of all the inner txs that have been
		// successfully executed when handling the wrapper. These

		/// The masp tx kind, fee-payment or transfer
		pub kind: MaspEventKind,

	/// An indexed masp tx carrying information on wether it was a fee paying tx or
	/// An indexed masp tx carrying information on whether it was a fee paying tx or

Conversation

grarco commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Checklist before merging

Uh oh!

codecov bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sug0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grarco Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grarco Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grarco Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grarco commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

grarco commented Mar 5, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading

grarco Mar 11, 2025 •

edited

Loading

grarco Mar 11, 2025 •

edited

Loading

grarco Mar 11, 2025 •

edited

Loading