-
Notifications
You must be signed in to change notification settings - Fork 155
Zero-fee commitments support #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e62de06
283e328
0b1594a
31aaba9
d230d93
1ee8883
59e5ec9
3ae4384
7778dfc
7fbfecb
097f9ba
cba213e
77af309
534a47c
1c0cef8
f0b2bc3
034779c
80e2d0b
f33ea14
700d3ba
56afedd
e889370
d8313f0
9ab37b5
5f0c38f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand on why this needs to be a separate script now? Why not keep using the old script that sets up both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex: This matches my original intention when I first created this standalone script for the expensive electrs build step. It looks like we want to avoid building the binary ourselves anyway for easier local development, so I will likely be able to consolidate things again once a binary that supports
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Okay, yeah, not too strong of an opinion.
Well, given how close this PR is it would be nice to land it for 0.8 still, and not wait indefinitely. Hmm, given that the Forgejo migration is already around the corner, further CI changes will be necessary anyways soon. Maybe we could at least provide a convenience wrapper script here, that sets up bitcoind/electrsd and runs the tests? Or even better just do #660 (comment), i.e., leave the current test setup as is and only test 0fc in a separate new file and workflow, so we don't require the custom electrs for all local tests? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #!/bin/bash | ||
|
benthecarman marked this conversation as resolved.
|
||
| set -eox pipefail | ||
|
|
||
| # Our Esplora-based tests require `electrs` binaries. Here, we | ||
| # download the code, build the binaries, and export their location | ||
| # via `ELECTRS_EXE` which will be used by the `electrsd` crates in | ||
| # our tests. | ||
|
|
||
| HOST_PLATFORM="$(rustc --version --verbose | grep "host:" | awk '{ print $2 }')" | ||
| ELECTRS_GIT_REPO="https://github.com/tankyleo/blockstream-electrs.git" | ||
| ELECTRS_TAG="2026-05-26-electrum-submit-package" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do this, we should use a specific commit revision, not point to a general branch.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is indeed my intention, and I believe the script currently does this. See the tag here: https://github.com/tankyleo/blockstream-electrs/releases/tag/2026-05-26-electrum-submit-package
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sorry, I took
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done below, the tag remains useful to do a |
||
| ELECTRS_REV="8c06d8010e43f793b1a65f83695ea846e5cd83ed" | ||
| if [[ "$HOST_PLATFORM" != *linux* && "$HOST_PLATFORM" != *darwin* ]]; then | ||
| printf "\n\n" | ||
| echo "Unsupported platform: $HOST_PLATFORM Exiting.." | ||
| exit 1 | ||
| fi | ||
|
|
||
| DL_TMP_DIR=$(mktemp -d) | ||
| trap 'rm -rf -- "$DL_TMP_DIR"' EXIT | ||
|
|
||
| pushd "$DL_TMP_DIR" | ||
| git clone --branch "$ELECTRS_TAG" --depth 1 "$ELECTRS_GIT_REPO" blockstream-electrs | ||
| cd blockstream-electrs | ||
| CURRENT_HEAD=$(git rev-parse HEAD) | ||
| if [ "$CURRENT_HEAD" != "$ELECTRS_REV" ]; then | ||
| echo "ERROR: HEAD does not match expected commit" | ||
| echo "expected: $ELECTRS_REV" | ||
| echo "actual: $CURRENT_HEAD" | ||
| exit 1 | ||
| fi | ||
| RUSTFLAGS="" cargo build | ||
| export ELECTRS_EXE="$DL_TMP_DIR"/blockstream-electrs/target/debug/electrs | ||
| chmod +x "$ELECTRS_EXE" | ||
| popd | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we add cargo cleans? won't that screw up the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hitting a "disk space exhausted error" on that job in CI, wanted to see if this helps, at the cost of individual CI taking longer yes.
cargo buildandcargo testseem to rebuild from scratch, no shared artifacts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume this is because we are building electrs in CI. Since its cached now, can you remove the cargo cleans? Maybe just put a clean after you upload the electrs binary