Stacks Watcher Update (Full changes) #4592
Conversation
* chore: wip * chore: wip * chore: wip pre-polling * feat: update to polling style * feat: add reobserve * docs: update comment * chore: remove port remnants * fix: update event logic and contracts * chore: remove unwanted changes * chore: clean up test dir * docs: update readme * chore: remove contract files for later merging in via separate pr * Revert "chore: remove contract files for later merging in via separate pr" This reverts commit 0e3fdaf. * fix: add more integration progress * chore: ai progress * chore: update readme * chore: update readme * chore: remove file * chore: wip * chore: update scripts for spy test * fix: add tx_status check for processing stacks transactions in the watcher * chore: remove outdated test dir (wormhole-foundation#6) Co-authored-by: janniks <janniks@users.noreply.github.com> * refactor(audit): minor updates (wormhole-foundation#7) * build: remove guardian dependency from stacks node * refactor: make default parameters configurable --------- Co-authored-by: janniks <janniks@users.noreply.github.com> * fix: update contracts * fix: ntt eth deploy updates * chore: rename tmp notes * fix: cherry pick watcher updates * chore: update parameters * fix(audit): update emitter address * fix: pull in updates * fix(audit): switch to state contract tracked owner var * fix: update state contract changes * chore: update notes * fix(audit): add active contract fetching * fix(audit): add max bigint checks, closes #4 * fix(audit): add missing fetch method * fix(audit): fix incorrect clarity byte * fix(audit): check burn block confirmation height for reobserve requests, closes #2 * fix: switch to confirmed height rather than processed height * fix: remove stacks port remnant * fix: add stacks defaults directly in node config * chore: remove addressed comments, l1 finalizers not needed for stacks * chore: update contracts * fix: add missing c32 file * added POC for stacks native api * fixed c32 * reverted variable renaming * test: update integration style tests * refactor: move to Tilt tests with it cases * refactor: update stacks test setup * refactor: update stacks k8s * chore: update test files * test: update spy handling * chore: update contracts * test: fix resolve check * chore: update logs * fix: remove submodule * fix: remove submodule * fix: update integration tests * fix: switch to latest node develop branch * fix: type errors * chore: remove old notes * refactor: rename native mode * chore: udpate to v3 endpoints * fix: add working hacknet * fix: update contracts * fix: cleanup config files * fix: add working nakamoto tx hacknet * fix: working wormhole message with node only * fix: minor updates * fix: update failure checks * refactor: streamline VAA monitoring * test: update integration test * test: add nonce to contract name * test: fix faulty tests * test: use pr build for stacks node --------- Co-authored-by: janniks <janniks@users.noreply.github.com> Co-authored-by: Roberto De Ioris <roberto.deioris@gmail.com> Co-authored-by: jannik-stacks <237011140+jannik-stacks@users.noreply.github.com>
* feat: squashed stacks implementation * chore: wip * chore: wip * chore: wip pre-polling * feat: update to polling style * feat: add reobserve * docs: update comment * chore: remove port remnants * fix: update event logic and contracts * chore: remove unwanted changes * chore: clean up test dir * docs: update readme * chore: remove contract files for later merging in via separate pr * Revert "chore: remove contract files for later merging in via separate pr" This reverts commit 0e3fdaf. * fix: add more integration progress * chore: ai progress * chore: update readme * chore: update readme * chore: remove file * chore: wip * chore: update scripts for spy test * fix: add tx_status check for processing stacks transactions in the watcher * chore: remove outdated test dir (wormhole-foundation#6) Co-authored-by: janniks <janniks@users.noreply.github.com> * refactor(audit): minor updates (wormhole-foundation#7) * build: remove guardian dependency from stacks node * refactor: make default parameters configurable --------- Co-authored-by: janniks <janniks@users.noreply.github.com> * fix: update contracts * fix: ntt eth deploy updates * chore: rename tmp notes * fix: cherry pick watcher updates * chore: update parameters * fix(audit): update emitter address * fix: pull in updates * fix(audit): switch to state contract tracked owner var * fix: update state contract changes * chore: update notes * fix(audit): add active contract fetching * fix(audit): add max bigint checks, closes #4 * fix(audit): add missing fetch method * fix(audit): fix incorrect clarity byte * fix(audit): check burn block confirmation height for reobserve requests, closes #2 * fix: switch to confirmed height rather than processed height * fix: remove stacks port remnant * fix: add stacks defaults directly in node config * chore: remove addressed comments, l1 finalizers not needed for stacks * chore: update contracts * fix: add missing c32 file * added POC for stacks native api * fixed c32 * reverted variable renaming * test: update integration style tests * refactor: move to Tilt tests with it cases * refactor: update stacks test setup * refactor: update stacks k8s * chore: update test files * test: update spy handling * chore: update contracts * test: fix resolve check * chore: update logs * fix: remove submodule * fix: remove submodule * fix: update integration tests * fix: switch to latest node develop branch * fix: type errors * chore: remove old notes * refactor: rename native mode * chore: udpate to v3 endpoints * fix: add working hacknet * fix: update contracts * fix: cleanup config files * fix: add working nakamoto tx hacknet * fix: working wormhole message with node only * fix: minor updates * fix: update failure checks * refactor: streamline VAA monitoring * test: update integration test * test: add nonce to contract name * test: fix faulty tests * test: use pr build for stacks node --------- Co-authored-by: janniks <janniks@users.noreply.github.com> Co-authored-by: Roberto De Ioris <roberto.deioris@gmail.com> Co-authored-by: jannik-stacks <237011140+jannik-stacks@users.noreply.github.com> * chore: remove unwanted file * fix: update pr according to repo guidelines * chore: revert go.mod * fix: pin dockerfiles * chore: make linter happy * chore: revert gitignore * fix: add txid to message pub * chore: cleanup duplicate go mod * chore: undo package change * chore: repin dockerfiles * chore: add lock files * chore: add lockfiles in dockerfile * refactor: make success check more verbose * refactor: ignore uncommitted events * fix: add reobservation flag for stacks message pubs * fix: remove unused l1finalizers * refactor: fix lint errors * docs: update stacks readmes * chore: rename stacks test packages --------- Co-authored-by: janniks <6362150+janniks@users.noreply.github.com> Co-authored-by: janniks <janniks@users.noreply.github.com> Co-authored-by: Roberto De Ioris <roberto.deioris@gmail.com>
…foundation/wormhole into ext-integration/stacks
* Parse None correctly. * Parse Signed integers with the signed bit. * Add length limits to dynamically sized values. * Add recursive depth limit * Use 'readFull()' instead of 'read()' on Buff * Allow empty buffers
|
@jannik-stacks it looks like this will take some additional work so that the Tilt tests can pass. In the meantime, we can review the main logic. |
johnsaigle
left a comment
There was a problem hiding this comment.
I left some initial feedback. However, it looks like this PR doesn't yet contain the full changes: it's missing @mdulin2 's fixes from the Stacks fork of Wormhole.
node/pkg/watchers/stacks/README.md
Outdated
|
|
||
| ## API Endpoints | ||
|
|
||
| The watcher interacts with the Stacks Node RPC API endpoints: |
There was a problem hiding this comment.
Could you add a link to official Stacks documentation here so that we can cross-reference?
stacks/Dockerfile
Outdated
| FROM rust@sha256:234b03300ecaae65919eb8cb13ca9b36588b8dff5b1c69581e361f9f25654db1 AS builder | ||
|
|
||
| ARG STACKS_CORE_REPO=https://github.com/stacks-network/stacks-core.git | ||
| ARG STACKS_CORE_BASE_BRANCH=develop |
There was a problem hiding this comment.
Could you change this to work from a specific commit hash, ideally one tied to a release? I think we'll want to use a fixed version rather than tracking the latest changes on develop.
# Conflicts: # devnet/stacks-bitcoin.yaml # devnet/stacks-signer.yaml # node/pkg/watchers/stacks/fetch.go # node/pkg/watchers/stacks/watcher.go # stacks/broadcaster/Dockerfile # stacks/stacker/Dockerfile # stacks/test/Dockerfile
# Conflicts: # node/pkg/watchers/stacks/clarity.go
* ci: run ci on this branch * ci: update runner * ci: increase mine interval * chore: trigger ci * ci: install tilt * ci: test k3d * ci: add local registry * ci: update timing for stacks devnet * chore: pin stacks node image * chore: undo ci changes
| continue | ||
| } | ||
|
|
||
| if len(hexBytes) > 1024*1024 { |
There was a problem hiding this comment.
Could you move this length check above the hex-decoding step? I think it makes sense to first check for an unbounded length before doing hex-decoding. Note you'll probably have to double the size of the check because each hex-encoded byte will be two characters vs. the decoded bytes here.
Could you also move this value to a constant so it's easier to track? Finally, if you do so, it would be good to log the actual constant value rather than hardcode (1MB) in the log. This will help the log stay accurate if the upper bound needs to changes in the future.
|
|
||
| // success | ||
|
|
||
| wormholeEvents := uint32(0) |
There was a problem hiding this comment.
This could maybe be a uint8 unless you're expecting a very large number of wormhole core events per transaction
| return fmt.Errorf("failed to fetch Stacks block replay: %w", err) | ||
| } | ||
|
|
||
| for _, tx := range replay.Transactions { |
There was a problem hiding this comment.
Do you know whether the JSON transactions field has stable ordering? I would assume it does. It might be worth adding a comment here because this loop will process transactions according to whatever order the Stacks node yields them.
| return fmt.Errorf("failed to fetch Stacks block replay: %w", err) | ||
| } | ||
|
|
||
| for _, tx := range replay.Transactions { |
There was a problem hiding this comment.
Is it worth adding a length check here? Are blocks always expected to have transactions? If so, then we might want to return an error here.
| zap.String("bitcoin_block_hash", tenureBlocks.BurnBlockHash)) | ||
|
|
||
| // Process each Stacks block anchored to this burn block | ||
| for _, block := range tenureBlocks.StacksBlocks { |
There was a problem hiding this comment.
Worth adding a length check on tenureBlocks.StacksBlocks?
| AnchorBlockTxid string `json:"anchor_block_txid"` | ||
| } | ||
|
|
||
| StacksV2InfoResponse struct { |
There was a problem hiding this comment.
Can these types be more strict? Are all of these really uint64 or are smaller types more accurate for decoding these responses? As mentioned above, a link to documentation would be helpful here.
| } | ||
|
|
||
| if w.rpcAuthToken != "" { | ||
| req.Header.Set("Authorization", w.rpcAuthToken) |
There was a problem hiding this comment.
Can you clarify which of these endpoints need authentication and why?
It may be better to create an http client in the Watcher object itself and configure it with an API key rather than re-implementing a DefaultClient in each of these methods.
| /// HELPERS | ||
|
|
||
| // Extracts the event name from an event tuple | ||
| func extractEventName(eventTuple *clarity.Tuple) (string, error) { |
There was a problem hiding this comment.
Please add a nil check for this parameter.
| } | ||
|
|
||
| // Extracts core message fields from an event tuple | ||
| func extractMessageData(eventTuple *clarity.Tuple) (*MessageData, error) { |
There was a problem hiding this comment.
Please add a nil check for this parameter.
| } | ||
|
|
||
| payload, ok := payloadVal.(*clarity.Buffer) | ||
| if !ok || payload.Len() > 8192 { |
There was a problem hiding this comment.
Could you move the value 8192 into a constant and document that this is related to the Clarity transaction size limit?
It may be helpful to create a consts.go where the constants are stored for the Stacks package.
Stacks Watcher Update
Base
mainIncludes PRs
Additional changes