Skip to content
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

add bitcoind to devshell, and setenv BITCOIND_EXE #514

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

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.
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 13000966288

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 78.485%

Files with Coverage Reduction New Missed Lines %
payjoin/src/send/mod.rs 2 93.79%
Totals Coverage Status
Change from base Build 12968704432: -0.04%
Covered Lines: 3648
Relevant Lines: 4648

💛 - 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
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