imp(core): allow huckleberry events with a prefix #5541

srdtrk · 2024-01-08T11:28:31Z

Description

Event emissions on erroneous IBC application callbacks were removed due to the huckleberry security advisory. Since then, many users have been unable to debug their IBC applications effectively causing a considerable uptick in support requests across many chains.

This PR enables emission of events on erroneous IBC application callbacks by appending a prefix to all event attribute keys

closes: #5284

Commit Message / Changelog Entry

imp(core): allow huckleberry events with a prefix

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

codecov-commenter · 2024-01-08T12:22:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f9b918f) 81.19% compared to head (4f9acf3) 81.20%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5541      +/-   ##
==========================================
+ Coverage   81.19%   81.20%   +0.01%     
==========================================
  Files         197      197              
  Lines       15253    15268      +15     
==========================================
+ Hits        12384    12399      +15     
  Misses       2404     2404              
  Partials      465      465

Files	Coverage Δ
...les/apps/27-interchain-accounts/host/ibc_module.go	`84.12% <100.00%> (ø)`
modules/apps/transfer/ibc_module.go	`69.10% <100.00%> (ø)`
modules/core/keeper/msg_server.go	`67.00% <100.00%> (+0.57%)`	⬆️

colin-axner · 2024-01-08T15:35:51Z

modules/core/types/events.go

+		// no need to append the error attribute prefix to the event type because
+		// the event type is not associated to a value that can be misinterpreted
+		newEvents[i] = sdk.NewEvent(event.Type, newAttributes...)


can we elaborate on this reasoning? I feel it is safer to prefix the type as well. Is there a direct benefit to not prefixing the type?

I thought it might be easier for users to index events by module that emitted the event. In that it wouldn't break their pre-existing clients too much.

The reasoning isn't solid but what I meant was that event types are only used for indexing attributes by module. Event types aren't the key to some value, they are only used to retrieve the attributes.

But I'm open to prefixing this too

Hmm yea. My main concern is accidental misuse, that is using the event type to indicate an action occurred without caring about particular values. But I think you make a fair point that event types aren't the key to some value. I'm not too concerned about breaking pre-existing clients as pre-existing clients currently cannot access error events. If the primary use case here is simply for debugging, I'm not sure I see much benefit in not error prefixing the type defensively, but I don't feel strongly, just feel slightly uncomfortable allowing any surface area for misinterpretation of events

Yeah fair enough. Added the prefix here as well

modules/core/types/events.go

modules/core/keeper/msg_server.go

chatton

LGTM, this looks like a solid implementation, thank you for putting this together! I just had a few minor nits but otherwise looks great!

chatton · 2024-01-10T10:44:08Z

modules/core/keeper/msg_server_test.go

@@ -419,6 +445,14 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
 				// replay should not error as it is treated as a no-op
 				_, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), msg)
 				suite.Require().NoError(err)
+
+				if replay {


small nit, to be more consistent maybe we could make this a tc.replay, we seem to be mixing things like tc.expPass and standalone bools like replay.

modules/core/keeper/msg_server_test.go

charleenfei

Thanks for the quick changes!

colin-axner · 2024-01-10T15:07:25Z

modules/core/keeper/msg_server_test.go

@@ -57,15 +58,15 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
 			suite.Require().NoError(err)

 			packet = channeltypes.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
-		}, true, false},
+		}, true, false, false, false},


maybe we should open an issue to make these named args in a followup (4 booleans are hard to process what they indicate without a name)

modules/core/keeper/msg_server_test.go

colin-axner · 2024-01-10T15:27:04Z

modules/core/types/events.go

+		// no need to append the error attribute prefix to the event type because
+		// the event type is not associated to a value that can be misinterpreted
+		newEvents[i] = sdk.NewEvent(event.Type, newAttributes...)


Hmm yea. My main concern is accidental misuse, that is using the event type to indicate an action occurred without caring about particular values. But I think you make a fair point that event types aren't the key to some value. I'm not too concerned about breaking pre-existing clients as pre-existing clients currently cannot access error events. If the primary use case here is simply for debugging, I'm not sure I see much benefit in not error prefixing the type defensively, but I don't feel strongly, just feel slightly uncomfortable allowing any surface area for misinterpretation of events

colin-axner

Everything looks very great to me! My only hesitation is not prefixing the event type, which I have left my thoughts on. I'm happy to go with whichever direction you think is best

It might be a good idea to mention this change in behaviour to relayers in case it causes unexpected behaviour in their code (parsing an event for which the expected key they want is missing)

* feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) # Conflicts: # modules/core/keeper/msg_server.go # modules/core/keeper/msg_server_test.go

* feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) # Conflicts: # modules/core/keeper/msg_server.go

* feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109)

* feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items

…5573) * imp(core): allow huckleberry events with a prefix (#5541) * feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) # Conflicts: # modules/core/keeper/msg_server.go # modules/core/keeper/msg_server_test.go * fix: merge conflicts * ci: removed link checker --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com>

…5574) * imp(core): allow huckleberry events with a prefix (#5541) * feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) # Conflicts: # modules/core/keeper/msg_server.go # modules/core/keeper/msg_server_test.go * fix: merge conflicts * nit * ci: removed link checker --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com>

…5575) * imp(core): allow huckleberry events with a prefix (#5541) * feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) # Conflicts: # modules/core/keeper/msg_server.go * fix: merge conflicts * nit --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com>

…5576) * imp(core): allow huckleberry events with a prefix (#5541) * feat: initial impl * imp: moved event helpers to types and added tests * imp(testing): added mock events to mock module * test: added msg_server tests for application events * imp: converted suffix to prefix * docs: updated inline comments * imp: review item * test: review items * imp: review items (cherry picked from commit 2375109) * nit --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com>

srdtrk added 6 commits January 5, 2024 12:56

feat: initial impl

c53f913

imp: moved event helpers to types and added tests

688a9d9

imp(testing): added mock events to mock module

70e7cad

test: added msg_server tests for application events

bdbd9f6

Merge branch 'main' into serdar/issue#5284-error-event-suffix

88f430b

imp: converted suffix to prefix

f4caa14

srdtrk added the core label Jan 8, 2024

srdtrk requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, charleenfei, chatton and DimitrisJim as code owners January 8, 2024 11:28

docs: updated inline comments

da528e4

crodriguezvega added the backport-to-v8.1.x label Jan 8, 2024

Merge branch 'main' into serdar/issue#5284-error-event-suffix

b5f6ee0

srdtrk mentioned this pull request Jan 8, 2024

ICA host packet failure events not being emitted #5284

Closed

3 tasks

crodriguezvega added backport-to-v7.2.x labels Jan 8, 2024

srdtrk added 2 commits January 8, 2024 15:37

Merge branch 'main' into serdar/issue#5284-error-event-suffix

6a2bef0

Merge branch 'main' into serdar/issue#5284-error-event-suffix

07fca7e

colin-axner reviewed Jan 8, 2024

View reviewed changes

srdtrk and others added 5 commits January 9, 2024 11:49

Merge branch 'main' into serdar/issue#5284-error-event-suffix

78a1c60

imp: review item

1c7862f

Merge branch 'main' into serdar/issue#5284-error-event-suffix

43a03f3

Merge branch 'main' into serdar/issue#5284-error-event-suffix

986fc82

Merge branch 'main' into serdar/issue#5284-error-event-suffix

16a3312

chatton approved these changes Jan 10, 2024

View reviewed changes

srdtrk added 2 commits January 10, 2024 14:49

Merge branch 'main' into serdar/issue#5284-error-event-suffix

a30078b

test: review items

fae32e4

charleenfei approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into serdar/issue#5284-error-event-suffix

6106643

colin-axner reviewed Jan 10, 2024

View reviewed changes

colin-axner approved these changes Jan 10, 2024

View reviewed changes

srdtrk added 2 commits January 11, 2024 11:20

Merge branch 'main' into serdar/issue#5284-error-event-suffix

4b58d7f

imp: review items

4f9acf3

srdtrk changed the title ~~imp(core): allow huckleberry events with an attribute prefix~~ imp(core): allow huckleberry events with a prefix Jan 11, 2024

srdtrk merged commit 2375109 into main Jan 11, 2024
62 checks passed

srdtrk deleted the serdar/issue#5284-error-event-suffix branch January 11, 2024 09:54

This was referenced Jan 11, 2024

imp(core): allow huckleberry events with a prefix (backport #5541) #5573

Merged

imp(core): allow huckleberry events with a prefix (backport #5541) #5574

Merged

This was referenced Jan 11, 2024

imp(core): allow huckleberry events with a prefix (backport #5541) #5575

Merged

imp(core): allow huckleberry events with a prefix (backport #5541) #5576

Merged

colin-axner mentioned this pull request Jan 11, 2024

nit: remove stale comment #5577

Merged

9 tasks

swelf19 mentioned this pull request Feb 1, 2024

updated icatx docs neutron-org/neutron-docs#148

Merged

nicolaslara mentioned this pull request Feb 1, 2024

Update IBC to emit errors as events osmosis-labs/osmosis#7400

Merged

6 tasks

crodriguezvega mentioned this pull request Apr 11, 2024

imp(release/v8.2.x): emit host disabled event #6142

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp(core): allow huckleberry events with a prefix #5541

imp(core): allow huckleberry events with a prefix #5541

srdtrk commented Jan 8, 2024 •

edited

codecov-commenter commented Jan 8, 2024 •

edited

colin-axner Jan 8, 2024

srdtrk Jan 8, 2024

colin-axner Jan 10, 2024

srdtrk Jan 11, 2024 •

edited

chatton left a comment

chatton Jan 10, 2024

srdtrk Jan 10, 2024

charleenfei left a comment •

edited

colin-axner Jan 10, 2024

colin-axner Jan 10, 2024

colin-axner left a comment •

edited

imp(core): allow huckleberry events with a prefix #5541

imp(core): allow huckleberry events with a prefix #5541

Conversation

srdtrk commented Jan 8, 2024 • edited

Description

Commit Message / Changelog Entry

codecov-commenter commented Jan 8, 2024 • edited

Codecov Report

colin-axner Jan 8, 2024

Choose a reason for hiding this comment

srdtrk Jan 8, 2024

Choose a reason for hiding this comment

colin-axner Jan 10, 2024

Choose a reason for hiding this comment

srdtrk Jan 11, 2024 • edited

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

chatton Jan 10, 2024

Choose a reason for hiding this comment

srdtrk Jan 10, 2024

Choose a reason for hiding this comment

charleenfei left a comment • edited

Choose a reason for hiding this comment

colin-axner Jan 10, 2024

Choose a reason for hiding this comment

colin-axner Jan 10, 2024

Choose a reason for hiding this comment

colin-axner left a comment • edited

Choose a reason for hiding this comment

srdtrk commented Jan 8, 2024 •

edited

codecov-commenter commented Jan 8, 2024 •

edited

srdtrk Jan 11, 2024 •

edited

charleenfei left a comment •

edited

colin-axner left a comment •

edited