Fix OL ignoring withdrawal fulfillments #821

delbonis · 2025-05-22T21:05:23Z

Description

See STR-1415.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Unsure if this actually fixes the issue.

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added (where necessary) tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

codecov · 2025-05-22T21:11:01Z

Codecov Report

Attention: Patch coverage is 7.69231% with 24 lines in your changes missing coverage. Please review.

Please upload report for BASE (releases/0.2.0@9a568c5). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/chaintsn/src/transition.rs	10.00%	18 Missing ⚠️
crates/state/src/state_op.rs	0.00%	6 Missing ⚠️

@@                Coverage Diff                @@
##             releases/0.2.0     #821   +/-   ##
=================================================
  Coverage                  ?   52.32%           
=================================================
  Files                     ?      298           
  Lines                     ?    32842           
  Branches                  ?        0           
=================================================
  Hits                      ?    17183           
  Misses                    ?    15659           
  Partials                  ?        0

Files with missing lines	Coverage Δ
crates/state/src/state_op.rs	`30.69% <0.00%> (ø)`
crates/chaintsn/src/transition.rs	`42.78% <10.00%> (ø)`

🚀 New features to boost your workflow:

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

github-actions · 2025-05-22T21:36:37Z

Commit: a67985e

SP1 Execution Results

program	cycles	success
Bitcoin Blockspace	71,442	✅
EVM EE STF	567,414	✅
CL STF	166,201	✅
Checkpoint	4,232	✅

crates/l1tx/src/filter/withdrawal_fulfillment.rs

crates/test-utils/src/bridge.rs

crates/l1tx/src/deposit/deposit_tx.rs

crates/l1tx/src/filter/withdrawal_fulfillment.rs

bin/strata-client/src/main.rs

delbonis · 2025-05-28T18:51:31Z

Okay I am not sure how to resolve this conflict, it seems like some nontrivial change from I'm not sure where. @storopoli

Rajil1213 · 2025-05-28T19:05:25Z

Okay I am not sure how to resolve this conflict, it seems like some nontrivial change from I'm not sure where

What do you mean? Looks like the changes are from the release/* branch?

storopoli · 2025-05-28T19:42:50Z

Okay I am not sure how to resolve this conflict, it seems like some nontrivial change from I'm not sure where. @storopoli

Wait, is this already on main? Because your target branch is releases/0.2.0. Is this intentional?

delbonis · 2025-05-29T15:22:19Z

@storopoli Should I be merging into main first and then backport?

storopoli · 2025-05-29T15:35:57Z

@storopoli Should I be merging into main first and then backport?

Yes!

storopoli · 2025-06-02T20:11:22Z

@delbonis heads up! alpenlabs/strata-bridge#154 was merged with alpn.

bewakes

minor nits

bin/strata-client/src/errors.rs

crates/l1tx/src/filter/types.rs

…inst possible panic

…ics with 4 byte magics

…tils crate

storopoli · 2025-06-09T17:13:37Z

@delbonis I've rebased this and set the target branch to main.
Let's first get the fix into main then we backport the squash commit that lands on main into releases-0.2.0.

As a heads up here MAGIC_BYTES is "ALPN" whereas in strata-bridge already merged is "alpn" (lowercase). Either we change here to lowercase or we merge this as UPPERCASE and I open a PR in strata-bridge to s/alpn/ALPN/ all over the place.

barakshani · 2025-06-09T17:45:25Z

As a heads up here MAGIC_BYTES is "ALPN"

Don't we take this value from rollup name?

storopoli · 2025-06-09T18:17:32Z

As a heads up here MAGIC_BYTES is "ALPN"

Don't we take this value from rollup name?

we have tag rollup_name da-tag all of these are header bytes that identify an OP_RETURN, hence MAGIC_BYTES.

barakshani · 2025-06-09T19:03:53Z

As a heads up here MAGIC_BYTES is "ALPN"

Don't we take this value from rollup name?

we have tag rollup_name da-tag all of these are header bytes that identify an OP_RETURN, hence MAGIC_BYTES.

I meant, if this is an externally provided parameter, why do we need to change something in this PR?

storopoli · 2025-06-09T19:09:25Z

I meant, if this is an externally provided parameter, why do we need to change something in this PR?

In this PR (and in strata-bridge) is mostly for tests and mocking stuff. It would be nice to have the same value in both repos.

crates/asm/common/src/tx.rs

crates/asm/stf/src/tx_filter.rs

crates/util/python-utils/src/constants.rs

delbonis · 2025-06-09T21:23:31Z

@storopoli

(in ref to)

As a heads up here MAGIC_BYTES is "ALPN"

This PR doesn't specify any value for the magic. It reads it from the rollup_name field. There is a constant in tests, but that's irrelevant for deployment.

delbonis · 2025-06-09T21:25:40Z

Oh

It would be nice to have the same value in both repos.

I actually think it's better if it's different so that we can make sure our code is reading from params consistently. We've had issues in the past where things were hardcoded that shouldn't have been.

storopoli

ACK 4deb76c

should be good to go now.
CI should pass with unit and functional tests.
The last 2 commits (3c4544b and 4deb76c) fixed them.

* refactor(chaintsn, state): add more logging of proto ops, protect against possible panic * refactor(l1tx): fix OP_RETURN magic parsing * refactor(l1tx): make OP_RETURN parsing more generic, fix tests * refactor(l1tx, util/python-utils): replace references to variable magics with 4 byte magics * refactor(l1tx, test-utils): move `create_opreturn_metadata` to test-utils crate * feat(strata-client): add startup check for rollup name len * docs(l1tx): fix typo * docs(strata-client): fix incomplete comment * chore: rebase fixes * chore: fix unit tests * chore: fix func tests --------- Co-authored-by: Jose Storopoli <[email protected]>

* Fix OL ignoring withdrawal fulfillments (#821) * refactor(chaintsn, state): add more logging of proto ops, protect against possible panic * refactor(l1tx): fix OP_RETURN magic parsing * refactor(l1tx): make OP_RETURN parsing more generic, fix tests * refactor(l1tx, util/python-utils): replace references to variable magics with 4 byte magics * refactor(l1tx, test-utils): move `create_opreturn_metadata` to test-utils crate * feat(strata-client): add startup check for rollup name len * docs(l1tx): fix typo * docs(strata-client): fix incomplete comment * chore: rebase fixes * chore: fix unit tests * chore: fix func tests --------- Co-authored-by: Jose Storopoli <[email protected]> * chore: update cargo.lock --------- Co-authored-by: Trey Del Bonis <[email protected]>

barakshani reviewed May 24, 2025

View reviewed changes

crates/l1tx/src/filter/withdrawal_fulfillment.rs Outdated Show resolved Hide resolved

storopoli reviewed May 26, 2025

View reviewed changes

crates/l1tx/src/filter/withdrawal_fulfillment.rs Show resolved Hide resolved

delbonis requested a review from storopoli May 27, 2025 18:51

storopoli reviewed May 27, 2025

View reviewed changes

crates/test-utils/src/bridge.rs Show resolved Hide resolved

barakshani reviewed May 27, 2025

View reviewed changes

crates/l1tx/src/deposit/deposit_tx.rs Outdated Show resolved Hide resolved

storopoli reviewed May 28, 2025

View reviewed changes

crates/l1tx/src/filter/withdrawal_fulfillment.rs Outdated Show resolved Hide resolved

barakshani reviewed May 28, 2025

View reviewed changes

bin/strata-client/src/main.rs Outdated Show resolved Hide resolved

delbonis marked this pull request as ready for review May 28, 2025 18:47

delbonis requested review from a team as code owners May 28, 2025 18:47

bewakes reviewed Jun 5, 2025

View reviewed changes

bin/strata-client/src/errors.rs Show resolved Hide resolved

crates/l1tx/src/filter/types.rs Show resolved Hide resolved

delbonis added 8 commits June 9, 2025 13:05

refactor(chaintsn, state): add more logging of proto ops, protect aga…

aff4f6b

…inst possible panic

refactor(l1tx): fix OP_RETURN magic parsing

3ccc87e

refactor(l1tx): make OP_RETURN parsing more generic, fix tests

e1c1de7

refactor(l1tx, util/python-utils): replace references to variable mag…

743d0c8

…ics with 4 byte magics

refactor(l1tx, test-utils): move create_opreturn_metadata to test-u…

bd56253

…tils crate

feat(strata-client): add startup check for rollup name len

6e2bfdf

docs(l1tx): fix typo

f56c407

docs(strata-client): fix incomplete comment

5719ca8

storopoli removed request for a team June 9, 2025 17:05

chore: rebase fixes

40341fd

storopoli force-pushed the STR-1415-fix-withdrawal-fulfillments branch from b64d026 to 40341fd Compare June 9, 2025 17:05

delbonis commented Jun 9, 2025

View reviewed changes

crates/asm/common/src/tx.rs Show resolved Hide resolved

crates/asm/stf/src/tx_filter.rs Show resolved Hide resolved

crates/util/python-utils/src/constants.rs Show resolved Hide resolved

storopoli added 2 commits June 10, 2025 15:25

chore: fix unit tests

3c4544b

chore: fix func tests

4deb76c

storopoli approved these changes Jun 10, 2025

View reviewed changes

storopoli self-assigned this Jun 10, 2025

storopoli requested review from Rajil1213, barakshani and bewakes June 10, 2025 19:13

barakshani approved these changes Jun 10, 2025

View reviewed changes

storopoli enabled auto-merge June 10, 2025 19:44

bewakes approved these changes Jun 11, 2025

View reviewed changes

storopoli added this pull request to the merge queue Jun 11, 2025

Merged via the queue into main with commit a14b185 Jun 11, 2025
21 checks passed

storopoli deleted the STR-1415-fix-withdrawal-fulfillments branch June 11, 2025 16:16

Fix OL ignoring withdrawal fulfillments #821

Fix OL ignoring withdrawal fulfillments #821

Uh oh!

Conversation

delbonis commented May 22, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

Uh oh!

codecov bot commented May 22, 2025

Codecov Report

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delbonis commented May 28, 2025

Uh oh!

Rajil1213 commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

storopoli commented May 28, 2025

Uh oh!

delbonis commented May 29, 2025

Uh oh!

storopoli commented May 29, 2025

Uh oh!

storopoli commented Jun 2, 2025

Uh oh!

bewakes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

storopoli commented Jun 9, 2025

Uh oh!

barakshani commented Jun 9, 2025

Uh oh!

storopoli commented Jun 9, 2025

Uh oh!

barakshani commented Jun 9, 2025

Uh oh!

storopoli commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delbonis commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delbonis commented Jun 9, 2025

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

delbonis commented May 22, 2025 •

edited by atlassian bot

Loading

github-actions bot commented May 22, 2025 •

edited

Loading

Rajil1213 commented May 28, 2025 •

edited

Loading

delbonis commented Jun 9, 2025 •

edited

Loading