Update bitcoind to v0.23.0

[rajarshimaitra]

Description

Fixes #598

Thanks @afilini for sugesting to update electrs version. That did the trick..

But unfortunately electrs v0.9.1 isn't working with bitcoind v0.22.0. So I have updated the backend to v0.23.0 too.

Also reported the create_wallet api inconsistency for bitcoincore-rpc here rust-bitcoin/rust-bitcoincore-rpc#225

Notes to the reviewers

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[rajarshimaitra]

Reported the problem to bitcoincore-rpc rust-bitcoin/rust-bitcoincore-rpc#225

[afilini]

Code looks good but there are a couple of tests timing out, were they working locally on your machine?

[rajarshimaitra]

Yes I can see them in local too.. Opened this up to start the discussion of fixing the rpc api.. There might be many things off with electrs and current bitcoin core.. I am on it to debug what else is going wrong..

[rajarshimaitra]

I am seeing weird intermittent test failure.. Sometimes the tests are blocking on something so they are getting timed out, and sometimes tests are throwing "resource unavailable" error..

---- blockchain::rpc::test::bdk_blockchain_tests::test_sync_receive_coinbase stdout ----
thread 'blockchain::rpc::test::bdk_blockchain_tests::test_sync_receive_coinbase' panicked at 'called `Result::unwrap()` on an `Err` value: JsonRpc(Transport(SocketError(Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" })))', src/testutils/blockchain_tests.rs:263:69

The errors are not consistent.. Different tests are erroring at different runs.. Still trying to figure whats going on..

[notmandatory]

I've been looking into the tests that failed in CI. Different tests are timing out when I run it locally but I suspect the causes are related, the one that I'm looking into that hangs on my local system is blockchain::electrum::test::bdk_blockchain_tests::test_sync_reorg_block:

RUST_LOG=trace cargo test --no-default-features --features test-electrum,verify blockchain::electrum::test::bdk_blockchain_tests::test_sync_reorg_block

For this one, after calling TestClient::invalidate it's not getting a response back from electrs in the wait_for_block, exponential_backoff_poll loop. The wait_for_block and exponential_backoff_poll does work correctly in TestClient::new after getting bitcoind to generate_to_address.

I'm going to keep trying to figure it out, but if anyone has any suggestions let me know.

[rajarshimaitra]

Thanks @notmandatory .. I have also witnessed almost of all of the test pausing in my local is causing by either wait_for_block or the wait_for_transaction function call.. For some reason electrs is not getting the required transaction or blocks to send back..

I am also honing into that something is going wrong in the electrs sync.. But I suspect in that case the bug might be lying deep inside electrs and its only occuring for electrs v0.9.1 and bitcoind v0.23.0..

ccing @RCasatta here if he can share any more suggestions to identify the bug..

[notmandatory]

I was able to get all the tests to pass except test_sync_reorg_block() for electrum by removing calls to block_headers_subscribe(), which now also returns the first header update, which is why the block_headers_pop() never returns a value (it was already returned on subscribe). Since wait_for_block() is only waiting for a particular height, I simplified it by just asking the electrum client for the block at that height in a loop until it's available.

notmandatory@b2c7b35

The test_sync_reorg_block() test for the electrum feature is now hanging on wallet.sync() after invalidating a block. But seems to work fine with the rpc feature.

[rajarshimaitra]

I was able to get all the tests to pass except test_sync_reorg_block() for electrum by removing calls to block_headers_subscribe()

Thanks.. That sounds like at least some progress. I haven't been able to get any closer. If you have the code handy with your change can you push into this PR?? I will try to see if I can dig there deeper and find something.

[notmandatory]

I made a little more progress today, but still not clear on a proper fix. What I found is that in the rust-electrum-client crate, in the raw_client::_reader_thread(..) function there's a loop and the below line is hanging waiting for input from the electrs server:

                    if let Err(e) = reader.read_line(&mut raw_resp) {

The hang seems to happen after sending a "blockchain.scripthash.get_history" request to the server, after invalidating a block on bitcoind. I tried adding a timeout to the electrum Client and it times out instead of hanging. But now I need to figure out if this is an electrs problem (it's not responding as it should), or a rust-electrum-client problem. Either way I don't see a quick fix. Do we need this PR for the next release?

[afilini]

Yes, I would like to fix this for the next release because Core 23.0 has been out for a while and we are still not compatible with it.

I can work on another PR to migrate our rpc client to use descriptor wallets, so that it can at least work with all the Core versions. For the time being we can continue testing against 22.0 and once we have a fix for the tests upgrade those to 23.0

[afilini]

block_headers_subscribe(), which now also returns the first header update, which is why the block_headers_pop() never returns a value (it was already returned on subscribe).

Are you saying that even if you subscribe to headers update you never receive any? Because that sounds like a bug in electrs

[notmandatory]

block_headers_subscribe(), which now also returns the first header update, which is why the block_headers_pop() never returns a value (it was already returned on subscribe).

Are you saying that even if you subscribe to headers update you never receive any? Because that sounds like a bug in electrs

No, you still get the header update but only in the result of the block_headers_subscribe() call, not again when you call block_headers_pop().

[afilini]

But the headers_subscribe call should not only send you the latest header immediately, it should also notify you when new blocks come in, without having to poll the server constantly.

So if you generate a new block on Core electrs should notify you and that notification should end up in the queue that block_headers_pop() tries to read from.. but the notification never gets there, so it hangs forever.

It's either electrs not sending it or rust-electrum-client missing it for some reason, which would be weird since it worked fine for months now.

Btw, if you run the code with debug log level rust-electrum-client should print all the raw messages coming in and out, so if electrs sends the notification you should see it there at least, as long as there's a thread reading from the tcp socket

[notmandatory]

The way the test_sync_reorg_block() currently works is it invalidates a block via bitcoind, then does a block_headers_subscribe and then polls in a loop on block_headers_pop. In this test we don't make any block changes between block_headers_subscribe and the block_headers_pop commands so I also wouldn't expect the headers_pop to return any data, but with electrs 0.8.10 it does otherwise the loop wouldn't end.

In newer versions of electrs one change is now electrs uses the bitcoind p2p protocol to find new block data instead of reading the blocks directly from the bitcoind block data files. I suspect this and other related code changed how the block_headers subscribe and pop work since with the new electrs 0.9.1 version the same commands are sent but we never get anything back from the pop. (I just re-tested this now in the release/0.19.0 branch with only changing electrs to 0.9.1 and enabling the bitcoind P2P:Yes when creating the test client).

[notmandatory]

I was able to fix the blockchain_tests wait_for_block() loop this way:

fn wait_for_block(&mut self, min_height: usize) {
        let mut header = self.electrsd.client.block_headers_subscribe().unwrap();
        loop {
            if header.height >= min_height {
                break;
            }
            header = exponential_backoff_poll(|| {
                self.electrsd.trigger().unwrap();
                self.electrsd.client.ping().unwrap();
                self.electrsd.client.block_headers_pop().unwrap()
            });
        }
    }

But then I get a different electrs problem where it receives a "blockchain.scripthash.get_history" but then returns no data back, which causes the test to hang on the next wallet.sync.

[afilini]

So I think the way you've patched wait_for_block is actually the right way to do it, before we were assuming we would call wait_for_block before electrs had a chance to process the new blocks, so we would always get the notification in the loop.

But with your changes we correctly account for the case where electrs has already processed the blocks and can send us the header right away.

The fact that get_history is empty is weird, it seems to imply that electrs sends the notification about new blocks before actually processing and/or indexing them. Can you try just adding a std::thread::sleep() somewhere after wait_for_block to see if that gives a chance to electrs to catch up? If this fixes it we'll have to rethink our code, maybe instead of waiting for blocks we should wait for txs to appear in the get_history for a specific script

[notmandatory]

I suspect there may be more wrong with electrs that could be related to what we're seeing with get_history. I manually started up in regtest mode bitcoind 0.22, electrs 0.9.7 and the python electrum 4.3.0a0 client. I then created a wallet in electrum and generated blocks with coins going to the new wallet. I confirmed a balance, and then used bitcoin-cli to invalidateblock the getbestblockhash. I think what should happen is electrs detects the reorg and then electrum updates the balance. But what happens is I get no messages in the electrs logs and the balance in the electrum wallet remains unchanged, even after waiting some time. I also tried sending a USR1 signal to the electrs PID but still nothing, and from what I can see in the code sending a USR1 signal isn't supposed to do anything since the 0.9.x releases.

I'm going to keep paying around with it.. and see if I can verify the correct behavior using a different electrum server like electrum personal server. If it looks like an issue with electrs I'll open an issue there and see if someone can confirm it.

I'm putting my testing notes here:
https://hackmd.io/@notmandatory/electrs-testing

[evanlinjin]

Reported the problem to bitcoincore-rpc rust-bitcoin/rust-bitcoincore-rpc#225

I tried fixing this in rust-bitcoin/rust-bitcoincore-rpc#230, however my changes seemed to have broken backwards-compatibility and I need to fix the tests.

[rajarshimaitra]

Thanks @evanlinjin for working on this.. Yes the API has to be changed there.. Let me know when you finalize the PR.. I will review and test it out..

[danielabrozzoni]

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!