Event observer: additional info for burn operations #3517

pavitthrap · 2023-01-20T21:11:55Z

Description

Adds a burnchain_op field to transaction payload for the event observer.

In addition, it makes the json encoding of each burnchain op type more friendly to read. Examples of the readable versions for each op can be seen in the test suite of src/chainstate/burn/operations/mod.rs.
It does this through the function blockstack_op_to_json.

Modified an existing test in neon_integrations.rs to ensure this field exists & is populated.

Applicable issues

fixes Send additional data to event observer for Stacks-over-BTC wire formats #2591

Additional info (benefits, drawbacks, caveats)

Leaving the names for the BurnchainHeaderHash serialization functions the same as the ones @kantai used in sBTC: Add burn ops RPC for PegIn #3511- in general I used all the same names/files he did so that conflicts would occur in a future merge & could be later be easily fixed.
There's one open concern I have around the serialization for PoxAddress. Right now, the serialization does not encode the optional AddressHashMode provided in a standard PoxAddress, meaning if this field is populated with Some, then when deserializing, it is set to None. I'm waiting for feedback from @kantai on this - I moved forward with this because this is what he did in sBTC: Add burn ops RPC for PegIn #3511, but just want to check all the same assumptions hold for the other burn ops that makes this lossiness okay.
In the future, I can consolidate some of serialization functions for the byte array types (like BurnchainHeader, BlockHeaderHash, VRFSeed). Right now the ser/deser functions are pretty much identical for a few types so it feels redundant.
- I could create a hex trait associated with impl_array_hexstring_fmt, and move to_hex and from_hex to that trait. Then, I can create a serialization/deserialization function for a trait object. This would be done to prevent having to create serialization functions for types that are identical under the hood.

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

codecov · 2023-01-23T20:38:13Z

Codecov Report

Merging #3517 (dab5d0b) into develop (37c7e12) will increase coverage by 34.14%.
The diff coverage is 52.38%.

@@             Coverage Diff              @@
##           develop    #3517       +/-   ##
============================================
+ Coverage    31.41%   65.55%   +34.14%     
============================================
  Files          298      240       -58     
  Lines       274979   130905   -144074     
============================================
- Hits         86381    85819      -562     
+ Misses      188598    45086   -143512

Impacted Files	Coverage Δ
src/chainstate/burn/db/sortdb.rs	`81.70% <ø> (+41.18%)`	⬆️
testnet/stacks-node/src/tests/neon_integrations.rs	`85.17% <0.00%> (-2.17%)`	⬇️
src/chainstate/stacks/db/blocks.rs	`68.89% <50.00%> (+35.44%)`	⬆️
src/chainstate/stacks/events.rs	`66.66% <50.00%> (ø)`
src/chainstate/burn/operations/mod.rs	`45.67% <62.66%> (+14.90%)`	⬆️
testnet/stacks-node/src/event_dispatcher.rs	`83.63% <100.00%> (+0.11%)`	⬆️
stx-genesis/build.rs	`0.00% <0.00%> (-94.45%)`	⬇️
build.rs	`0.00% <0.00%> (-82.70%)`	⬇️
testnet/stacks-node/src/tests/epoch_21.rs	`88.68% <0.00%> (-7.45%)`	⬇️
clarity/src/vm/version.rs	`25.00% <0.00%> (-3.58%)`	⬇️
... and 193 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

docs/event-dispatcher.md

testnet/stacks-node/src/event_dispatcher.rs

kantai · 2023-01-25T19:53:25Z

@zone117x -- in the issue #2591, you mentioned that the API needs more information from events for things like the stack-stx operation to be processed correctly. This PR doesn't alter the "transaction events" portion of the event notifications, does that still need to be done, or did other changes to the PoX events solve that issue?

zone117x · 2023-01-26T12:01:03Z

@zone117x -- in the issue #2591, you mentioned that the API needs more information from events for things like the stack-stx operation to be processed correctly. This PR doesn't alter the "transaction events" portion of the event notifications, does that still need to be done, or did other changes to the PoX events solve that issue?

I think the new PoX-2 contract events do include the data needed, although I haven't yet looked into using it for the stack-stx bitcoin op parsing code. Let me give that a try and get back to you.

EDIT: yep, the stack-stx bitcoin-op can now be correctly parsed with the new pox-2 events hirosystems/stacks-blockchain-api#1533

jcnelson

Can you add some unit tests to each of the burnchain ops to verify that the JSON representation is accurate (i.e. conforms to the desired schema)? That's basically the only thing standing in the way of my approval right now.

jcnelson · 2023-01-27T20:01:24Z

Also, can you rebase this on develop? That's where 2.1 lives now.

jcnelson

LGTM; just need two things to be done before merging:

This needs to be based on develop, not next.
This should extend @kantai's work on sBTC: Add burn ops RPC for PegIn #3511 to use the Rust-idiomatic way of serializing burnchain operations to strings.

CLAassistant · 2023-01-31T20:00:57Z

All committers have signed the CLA.

The base branch was changed.

…tc ops is readable

src/chainstate/burn/operations/mod.rs

jcnelson

LGTM. Just a minor code factoring nit about putting serialize/deserialize decorator methods into a common file in stacks-common.

docs/event-dispatcher.md

kantai

These changes look good to me, but I think @zone117x and @jcnelson's comments should addressed. These changes impact the sortitiondb and burndb serialization and deserialization; I think this is fine, though, because 2.1 is already modifying these schemas enough that nodes need to sync from genesis anyways.

However, it does look like the serialization and deserialization changes broke some of the stack-stx tests, so it seems like something there needs to be fixed.

jcnelson · 2023-02-02T21:07:29Z

src/chainstate/stacks/address.rs

@@ -92,6 +94,49 @@ pub enum PoxAddress {
    Addr32(bool, PoxAddressType32, [u8; 32]),
 }

+/// Serializes a PoxAddress as a B58 check encoded address or a bech32 address
+pub fn pox_addr_b58_serialize<S: Serializer>(


These methods don't preserve the AddressHashMode, which makes them unsuitable for use within the sortition DB.

I think you have two options:

You can update the serializers for PoxAddress so that the relevant metadata is preserved, or

You can create a JSON-specific serialization method that will be used for the event stream.

I'm more in favor of the latter since, selfishly, I'm in the process of using already-spun-up nodes for testing and don't want to have to sync from genesis again, but that's something I can work around on my end. Up to you how you want to proceed. If you take the latter, please document it.

Just implemented the latter!

I added docs for the serialization in the event dispatcher doc already.

docs/event-dispatcher.md

src/chainstate/burn/operations/mod.rs

kantai

Looks good to me!

src/chainstate/burn/operations/mod.rs

blockstack-devops · 2024-11-14T00:20:34Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pavitthrap added the stacks-future label Jan 20, 2023

pavitthrap requested review from kantai and zone117x January 20, 2023 21:11

pavitthrap marked this pull request as draft January 20, 2023 21:12

pavitthrap force-pushed the btc-tx-more-info branch 2 times, most recently from 27a6f34 to 8f1e5d6 Compare January 20, 2023 21:34

pavitthrap marked this pull request as ready for review January 23, 2023 20:56

zone117x reviewed Jan 24, 2023

View reviewed changes

docs/event-dispatcher.md Show resolved Hide resolved

kantai reviewed Jan 25, 2023

View reviewed changes

testnet/stacks-node/src/event_dispatcher.rs Outdated Show resolved Hide resolved

jcnelson reviewed Jan 27, 2023

View reviewed changes

jcnelson added stacks-2.1 2.1-rc4 labels Jan 31, 2023

jcnelson requested review from jcnelson, kantai and zone117x January 31, 2023 16:30

jcnelson previously approved these changes Jan 31, 2023

View reviewed changes

pavitthrap force-pushed the btc-tx-more-info branch from 1a39f3f to 3893a52 Compare January 31, 2023 20:00

pavitthrap changed the base branch from next to develop January 31, 2023 22:01

pavitthrap added 5 commits February 1, 2023 17:02

added burnchain_op field to transaction event payload + test

57436f0

changed parameter name

06cad4a

update event dispatcher md file

441ad8f

wip - add example of stx op to md file

3d2b3e5

added docs, serialization functions, tests, and json formatting for b…

f36473a

…tc ops is readable

pavitthrap force-pushed the btc-tx-more-info branch from 3893a52 to f36473a Compare February 1, 2023 22:03

pavitthrap and others added 2 commits February 1, 2023 17:10

updated address hash mode

ebeeede

Merge branch 'develop' into btc-tx-more-info

998a7a4

pavitthrap requested a review from jcnelson February 2, 2023 03:40

jcnelson reviewed Feb 2, 2023

View reviewed changes

src/chainstate/burn/operations/mod.rs Outdated Show resolved Hide resolved

jcnelson approved these changes Feb 2, 2023

View reviewed changes

zone117x reviewed Feb 2, 2023

View reviewed changes

docs/event-dispatcher.md Outdated Show resolved Hide resolved

zone117x reviewed Feb 2, 2023

View reviewed changes

docs/event-dispatcher.md Outdated Show resolved Hide resolved

zone117x reviewed Feb 2, 2023

View reviewed changes

docs/event-dispatcher.md Outdated Show resolved Hide resolved

kantai requested changes Feb 2, 2023

View reviewed changes

jcnelson and others added 2 commits February 2, 2023 17:13

Merge branch 'develop' into btc-tx-more-info

8cdf09d

addressed feedback

b3fe8d4

jcnelson reviewed Feb 2, 2023

View reviewed changes

pavitthrap added 2 commits February 2, 2023 17:16

added specific json serialization function

487747a

cleanup

2b9565f

jcnelson reviewed Feb 3, 2023

View reviewed changes

docs/event-dispatcher.md Show resolved Hide resolved

jcnelson reviewed Feb 3, 2023

View reviewed changes

src/chainstate/burn/operations/mod.rs Show resolved Hide resolved

added docs, updated field names in json object

9911a8d

kantai approved these changes Feb 3, 2023

View reviewed changes

zone117x reviewed Feb 3, 2023

View reviewed changes

src/chainstate/burn/operations/mod.rs Outdated Show resolved Hide resolved

switched from pascal case to camel case

dab5d0b

jcnelson merged commit ecfe80b into develop Feb 3, 2023

kantai mentioned this pull request Feb 17, 2023

Add burnchain_op info to event emitter hirosystems/stacks-subnets#216

Merged

2 tasks

blockstack-devops added the locked label Nov 14, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event observer: additional info for burn operations #3517

Event observer: additional info for burn operations #3517

pavitthrap commented Jan 20, 2023 •

edited

Loading

codecov bot commented Jan 23, 2023 •

edited

Loading

kantai commented Jan 25, 2023

zone117x commented Jan 26, 2023 •

edited

Loading

jcnelson left a comment

jcnelson commented Jan 27, 2023

jcnelson left a comment

CLAassistant commented Jan 31, 2023 •

edited

Loading

jcnelson left a comment

kantai left a comment

jcnelson Feb 2, 2023

pavitthrap Feb 2, 2023

pavitthrap Feb 2, 2023

kantai left a comment

blockstack-devops commented Nov 14, 2024

Event observer: additional info for burn operations #3517

Event observer: additional info for burn operations #3517

Conversation

pavitthrap commented Jan 20, 2023 • edited Loading

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

kantai commented Jan 25, 2023

zone117x commented Jan 26, 2023 • edited Loading

jcnelson left a comment

Choose a reason for hiding this comment

jcnelson commented Jan 27, 2023

jcnelson left a comment

Choose a reason for hiding this comment

CLAassistant commented Jan 31, 2023 • edited Loading

jcnelson left a comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

jcnelson Feb 2, 2023

Choose a reason for hiding this comment

pavitthrap Feb 2, 2023

Choose a reason for hiding this comment

pavitthrap Feb 2, 2023

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 14, 2024

pavitthrap commented Jan 20, 2023 •

edited

Loading

codecov bot commented Jan 23, 2023 •

edited

Loading

zone117x commented Jan 26, 2023 •

edited

Loading

CLAassistant commented Jan 31, 2023 •

edited

Loading