Skip to content

Drop electrsd's auto-download feature for good #3020

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

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Apr 25, 2024

... which requires a bunch of unnecessary dev dependencies, e.g., zip.

Instead we lean on the download_bitcoind_electrs.sh script also for local testing.

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch 3 times, most recently from 2cad4fb to baf30a6 Compare April 25, 2024 13:34
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.73%. Comparing base (9de7c1d) to head (4320f48).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3020      +/-   ##
==========================================
- Coverage   89.73%   89.73%   -0.01%     
==========================================
  Files         122      122              
  Lines      101921   101922       +1     
  Branches   101921   101922       +1     
==========================================
  Hits        91463    91463              
- Misses       7770     7773       +3     
+ Partials     2688     2686       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch 2 times, most recently from eeec14d to 30c110b Compare April 25, 2024 13:45
@TheBlueMatt
Copy link
Collaborator

Thanks! Now that we have a script, can we run download_bitcoind_electrs.sh directly from git CI yaml and then cache the outputs? That way we can avoid wasting all of bitcoincore.org's bandwidth.

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch from 30c110b to 4c492d8 Compare August 1, 2024 15:47
@TheBlueMatt
Copy link
Collaborator

Any update here, looks like we currently can't build due to an MSRV bump in the time crate which this would fix.

@tnull
Copy link
Contributor Author

tnull commented Aug 9, 2024

Any update here, looks like we currently can't build due to an MSRV bump in the time crate which this would fix.

Uh, good to know. No update so far, but then I'll see to prioritize it (likely start of next week).

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch 14 times, most recently from 415aebb to d341821 Compare August 12, 2024 10:47
@tnull
Copy link
Contributor Author

tnull commented Aug 12, 2024

@TheBlueMatt I think this should be ready for review. So far it seems to work.

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch 2 times, most recently from 3c63c7d to f3cc788 Compare August 12, 2024 12:59
@tnull
Copy link
Contributor Author

tnull commented Aug 12, 2024

Mh, seems so far it didn't save the cached files, lets see if it does with the pushed fixup..

@TheBlueMatt
Copy link
Collaborator

Is it possible it only caches when running on a merge and not on PRs?

@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch from f3cc788 to 4eed83d Compare August 12, 2024 15:07
@tnull
Copy link
Contributor Author

tnull commented Aug 12, 2024

Is it possible it only caches when running on a merge and not on PRs?

I thought that first, too, but no, it also runs on PRs (cf. https://github.com/lightningdevkit/ldk-node/actions/caches). Actually all caches are branch-specific (so also of limited usefulness, tbh.). I previously included $HOME in path and that somehow didn't seem to work, but the fix that worked on LDK Node somehow seems to break CI here, so I'll need a few more tries to get this right I guess.

@TheBlueMatt
Copy link
Collaborator

so I'll need a few more tries to get this right I guess.

Alright, let us know when its there :)

tnull added 6 commits August 13, 2024 08:50
... allowing it to be sourced locally before running
`lightning-transaction-sync` tests.
... which requires a bunch of unnecessary dev dependencies, e.g., `zip`.

Instead we lean on the `download_bitcoind_electrs.sh` script also for
local testing.
@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch 2 times, most recently from 8f9b9c8 to 8ebf00b Compare August 13, 2024 08:39
@tnull
Copy link
Contributor Author

tnull commented Aug 13, 2024

Alright, let us know when its there :)

Alright, a thousand fixups and force-pushes later, this should be ready.

@tnull tnull requested a review from TheBlueMatt August 13, 2024 10:32
@tnull tnull force-pushed the 2024-04-drop-electrsd-autodownload-for-good branch from 8ebf00b to 4320f48 Compare August 13, 2024 13:02
cargo test --verbose --color always --features electrum
cargo check --verbose --color always --features electrum
popd
if [ -z "$BITCOIND_EXE" ] || [ -z "$ELECTRS_EXE" ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this exception to still allow running ./ci/ci-tests.sh locally without necessarily running the download script before. However, in CI we may want to fail here to avoid skipping tests if something with our CI setup is flaky.

@TheBlueMatt Do you have an opinion if it would be worth handing in a CI flag for this, or if we're fine skipping here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yea, would be nice to have a CI flag, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a follow-up: #3240

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All just CI changes that are straightforward. More importantly, I need this to actually test things locally (cause I use MSRV often), so gonna just land this.

@TheBlueMatt TheBlueMatt merged commit fc21640 into lightningdevkit:main Aug 13, 2024
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants