Error fetching script balance from current electrs server

[sgeisler]

There seem to be two different kinds of electrum server implementations out there (or it's just a bug) when it comes to the result of script_get_balance:

• one (electrum.blockstream.info) returns a big positive number for outgoing unconfirmed value such that 2^64-value==actual unconfirmed outgoing balance, this might be an unpatched bug in an earlier electrs version, otoh most clients seem to cope with it, so maybe the standard is weird and abuses some overflow arithmetic?
• current electrs versions (e.g. mine at bcwat.ch) will return a negative unconfirmed balance instead (which seems like the sane thing to do, but produces an error in this library)

The problem is that rust-electrum-client assumes the former and crashes on receiving a response in the latter format (since the data type serde tries to parse is a u64).

To test this behavior you can take any address with an outgoing, unconfirmed transaction and run the following example:

extern crate bitcoin;
extern crate electrum_client;

use bitcoin::Address;
use electrum_client::{Client, ConfigBuilder, ElectrumApi};
use std::str::FromStr;

fn main() {
    let addr = "1EQDBXLZUPDFiVNZMbsc4a6vkL3rp6haVQ"; // <- change this

    let client1 = Client::new("ssl://electrum.blockstream.info:50002").unwrap();
    let balance1 = client1
        .script_get_balance(&Address::from_str(addr).unwrap().script_pubkey())
        .unwrap();
    eprintln!("blockstream responded: {:?}", balance1);

    let cfg = ConfigBuilder::new().validate_domain(false).build();
    let client2 = Client::from_config("ssl://bcwat.ch:50002", cfg).unwrap();
    let balance2 = client2
        .script_get_balance(&Address::from_str(addr).unwrap().script_pubkey())
        .unwrap();
    eprintln!("bcwatch responded: {:?}", balance2);

    assert_eq!(balance1.unconfirmed, balance2.unconfirmed);
    assert_eq!(balance1.confirmed, balance2.confirmed);
}

It should crash somewhat like this:

blockstream responded: GetBalanceRes { confirmed: 26160000, unconfirmed: 18446744073683391616 }
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: AllAttemptsErrored([JSON(Error("invalid value: integer `-26160000`, expected u64", line: 0, column: 0))])', examples/unconfirmed_error.rs:21:10

The easy fix would be to make the unconfirmed balance a i64 and special casing e.g. any value over 18444644073709551616 (2^64-21*10^14). Maybe @romanz knows what's the right thing to do or how this situation came to be, was blockstream's version just forked before romaz/electrs@f387d038bc500bafaf44e08448aee25ef795a3d1 and nobody actually uses script_get_balance so no once noticed or was there a change in the protocol?

[afilini]

Thanks for the report! Most of this library has been developed by looking at the ElectrumX spec. so it makes sense that we might have missed something specific to electrs. Specifically the section that describes get_balance doesn't mention negative or very high values at all.

Anyway, if servers found "in the wild" already use this I guess we will have to change our type to i64 and add extra processing to detect very high values and do the required math.

[sgeisler]

It's super annoying, ideally we'd find out it's just a bug in the blockstream fork, deploy a fix and change it to i64 without any special casing. Relying on something that looks like it stems from undefined behavior is less than ideal.

[afilini]

CC-ing @shesek as well, I think he's done some work on the Blockstream fork.. let's see what they say and then figure out a solution

[shesek]

This was indeed a bug in the Electrum server implementation, fixed by Blockstream/electrs@bae4881. Will try to expedite a deployment for this.

The reason this went unnoticed was that its not actually used by the Electrum wallet itself, only for manual getaddressbalance calls via its cli/rpc interfaces.

[sgeisler]

@shesek Why would a confirmed balance ever be negative? That would indicate an error imo. It's not as if I didn't actually do the same when I fixed this bug for myself at first 😄 but it seems nonsensical other than maybe for doing away with casting when adding the both values.

[shesek]

You're absolutely right, I think it's because I saw romanz/electrs@f387d03 doing this too right before implementing the fix myself :) Changed in Blockstream/electrs@d1fd717.

[afilini]

Released in 0.7.0