Skip to content

Conversation

@nothingmuch
Copy link
Collaborator

This change supplies bitcoind from nixpkgs, and sets BITCOIND_EXE to its full path, for more convenient testing.

BITCOIND_SKIP_DOWNLOAD is also set, although that's only actually required in sandboxed builds.

Note that this decouples the bitcoind version from that set in the feature flags of the bitcoind crate, with the default version from nixpkgs being selected and locked in the flake.lock file. I haven't had any problems with it, and ideally tests should pass with all versions.

Also fixes formatting with nix fmt, which I forgot in the cargo-llvm-cov PR.

@coveralls
Copy link
Collaborator

coveralls commented Jan 28, 2025

Pull Request Test Coverage Report for Build 20097061482

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.176%

Totals Coverage Status
Change from base Build 20077534309: 0.0%
Covered Lines: 9665
Relevant Lines: 11620

💛 - Coveralls

@nothingmuch
Copy link
Collaborator Author

BTW another thing I often do for local development, although not really the topic of this PR I thought I'd mention it here, is nest another nix shell nixpkgs#sccache and set RUSTC_WRAPPER=sccache. If others use the dev shell stuff please lmk your thoughts about that, IME it's a bit slower for some things esp. failed builds, but seems very effective when doing things like git rebase --exec ./contrib/test_local.sh repeatedly, but I did not actually measure this stuff it's just vibes for now.

@DanGould DanGould self-requested a review January 28, 2025 16:01
@DanGould
Copy link
Contributor

If I'm in this nix develop shell is this supposed to supply bitcoind for tests by default? I'm getting this error for every integration test that requires bitcoind when I run ./contrib/test_local.sh. Do I need to run bitcoind manually?

---- integration::batching::receiver_forwards_payment stdout ----                                                                                   
Error: JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }))) 

@nothingmuch
Copy link
Collaborator Author

If I'm in this nix develop shell is this supposed to supply bitcoind for tests by default?

Yes. The inclusion of the bitcoind package on its own should not affect anything, but setting the environment variable should have that effect

Do I need to run bitcoind manually?

No, the BITCOIND_EXE environment variable just overrides the bitcoind crate supplied binary with a nixpkgs one but other than that it's supposed to work the same (i.e. the bitcoind crate is supposed to use that binary with a config supplied by TestServices).

I'm getting this error for every integration test that requires bitcoind when I run ./contrib/test_local.sh.

---- integration::batching::receiver_forwards_payment stdout ----                                                                                   
Error: JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }))) 

EWOULDBLOCK is really weird to me, not sure why anything would be using nonblocking IO flags for the JSON RPC stuff...

can you share and RUST_LOG=debug (bitcoind uses env_logger) output, and a also RUST_BACKTRACE=1 for the error if that's a crash, or if not try and localize it to where it arises?

@DanGould
Copy link
Contributor

Here's the requested log with RUST_LOG=debug RUST_BACKTRACE=1 ./contrib/test_local.sh
nix-bitcoind-mac-fail.log

I'm also perplexed by tests failing when I run just the v2 suite in this configuration:

running 5 tests
test integration::v2::test_bad_ohttp_keys ... ok
test integration::v2::v2_to_v1 ... ok
test integration::v2::test_session_expiration ... ok                                                                                                
test integration::v2::v1_to_v2 ... ok
test integration::v2::v2_to_v2 ... FAILED                        
                                                                          
failures:                                                                                                                                           
                                                                                                                                                    
---- integration::v2::v2_to_v2 stdout ----                                                                                                          
Database running on 127.0.0.1:55252                                                                                                                 
Directory server binding to port [::]:54786                         
Directory server binding to port [::]:54790
thread 'integration::v2::v2_to_v2' panicked at payjoin-test-utils/src/lib.rs:150:5:
assertion `left == right` failed: sender doesn't own bitcoin             
  left: 5000000000 SAT                                                    
 right: 505000000000 SAT                                                                                                                            
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace                                                                       
                                                                                                                                                    

failures:      
    integration::v2::v2_to_v2                                             
                                     
test result: FAILED. 4 passed; 1 failed; 0 ignored; 0 measured; 7 filtered out; finished in 26.12s                                                  
                                     
error: test failed, to rerun pass `-p payjoin --test integration`

It may be unrelated but I figured I should note it regardless

@nothingmuch nothingmuch marked this pull request as draft February 5, 2025 16:17
@spacebear21
Copy link
Collaborator

It would be great to revive this and get this in, which would also solve the Github issues when downloading bitcoind from the core releases:

error: failed to run custom build command for `bitcoind v0.36.1`

Caused by:
  process didn't exit successfully: `/home/runner/work/rust-payjoin/rust-payjoin/payjoin-ffi/target/debug/build/bitcoind-9a98c2f22ff992fd/build-script-build` (exit status: 101)
  --- stdout
  filename:bitcoin-0.21.2-x86_64-linux-gnu.tar.gz version:0.21.2 hash:67aab7c3dd32f7eb0f3d8ecd66f2da31c4cb969aec4c6a57cb8b01d22fe891e0

  --- stderr

  thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bitcoind-0.36.1/build.rs:9:23:
  called `Result::unwrap()` on an `Err` value: cannot reach url https://bitcoincore.org/bin/bitcoin-core-0.21.2/bitcoin-0.21.2-x86_64-linux-gnu.tar.gz

@nothingmuch
Copy link
Collaborator Author

rebased

This change supplies bitcoind from nixpkgs, and sets `BITCOIND_EXE` to
its full path, for more convenient testing.

`BITCOIND_SKIP_DOWNLOAD` is also set, although that's only actually
required in sandboxed builds.
@arminsabouri
Copy link
Collaborator

@nothingmuch ready for review?

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.

5 participants