misleading spendable balance

[zoedberg]

Describe the bug

I've noticed a strange behaviour of the get_balance API.

To Reproduce

Creating a new wallet and sending some funds to it produces the following balance:

spendable: 0
total: 10000
immature: 0
confirmed: 0
trusted pending: 0
untrusted pending: 10000

Since the spendable balance is 0 I assumed it was not possible to send funds, but still I've been able to spend the funds and bdk now reports the following balance:

spendable: 8859
total: 8859
immature: 0
confirmed: 0
trusted pending: 8859
untrusted pending: 0

Maybe this is not a bug and is just a matter of naming, but to me spendable should reflect the amount of funds that is possible to spend.

Expected behavior

I see that you documented spendable as the "sum of trusted_pending and confirmed coins", so IMO a more fitting name could be safe_to_spend balance.

Moreover we probably need also a spendable balance defined as the sum of funds that can be spent, that I think will be the sum of all balances except the immature one.

Build environment

• BDK tag/commit: v0.24.0
• OS+version: debian 11
• Rust/Cargo version: 1.65.0
• Rust/Cargo target: x86_64-unknown-linux-gnu

[danielabrozzoni]

Concept ACK for using safe_to_spend instead of spendable, you're right that it is way more clear!

[danielabrozzoni]

I had a chat with @hrik2001 and he's going to work on this one :)

[danielabrozzoni]

We just discussed this issue in bitcoindevkit/.github#35:

• safe_to_spend is superior in every way to spendable
• changing the definition of spendable "under the hood" is actually quite risky, as users using spendable might not notice
• we need to have timelocks in the balance as well (see Maybe we should mark which coins are confirmed but not yet spendable in the balance? #676)
• we have to avoid keep breaking the API at each release, as it's quite annoying

For these reasons, it's better if we delay fixing this issue until we have a clear path for #676, and at that point, fix both at the same time in BDK 1.0

[PoolloverNathan]

My vote goes to depreciating spendable making it an alias to a new field safe_to_spend, and adding possibly_safe_to_spend or something like that that has the above behavior.

[LLFourn]

note this is ready to fix in bdk_chain if @danielabrozzoni or anyone else has a vision for how it should work. Any problems with the current impl I no doubt ported over there!

here's the function: https://github.com/LLFourn/bdk_core_staging/blob/0fb4e1b20c4d31ecb785509311124d3fc4222902/bdk_chain/src/keychain/keychain_tracker.rs#L238

[danielabrozzoni]

I think we should keep a Balance structure, defined as:

pub struct Balance {
    immature: Amount,
    confirmed: Amount,
    trusted_pending: Amount,
    untrusted_pending: Amount,
}

On Balance we define these methods:
total() = immature + confirmed + trusted_pending + untrusted_pending
spendable() = confirmed + trusted_pending + untrusted_pending
safe_to_spend() = confirmed + trusted_pending

get_balance will return Balance, just as it does right now.

Then, regarding #676: I'm quite torn between having a get_spending_plan_balance(assets: Vec<Assets>) -> SpendingPlanBalance method or a get_policy_balance(policy: Policy) -> PolicyBalance method. The spending plan is engineered for transaction building, and I don't think it makes much sense here, on the other end, we noticed that users struggled with using the policies.

Either PolicyBalance or SpendingPlanBalance would be defined as

pub struct WhateverWePickBalance {
    locked: Amount,
    spendable: Balance
}

we could probably omit locked and just return a Balance without losing information, but I think it's clearer if we leave it.

Let's show with an example how both methods would work.

In both cases, we have the policy or(A,and(B,older(timelock)))
and
Utxo 1: unconfirmed, untrusted
Utxo 2: timelock+1 confirmations
Utxo 3: timelock-1 confirmations

Let's start with get_policy_balance: I honestly think this makes more sense, but you tell me:

get_policy_balance("or(A,and(B,older(timelock)))") and get_policy_balance("A") give the same result as get_balance:

SpendingPlanBalance {
    locked: 0
    spendable: Balance {
        immature: 0
        confirmed: utxo2+utxo3
        trusted_pending: 0
        untrusted_pending: utxo1
     }
}

While get_policy_balance("and(B, older(timelock))") only returns utxo2 as unspendable:

SpendingPlanBalance {
    locked: utxo1 + utxo3
    spendable: Balance {
        immature: 0
        confirmed: utxo2
        trusted_pending: 0
        untrusted_pending: 0
     }
}

Again, this seems pretty clear to me, but it does have all the drawbacks of the current Policy structure.

get_spending_plan_balance is instead pretty weird, as the timelock assets literally mean "I can wait up to [time]".
get_spending_plan_balance(A) returns

SpendingPlanBalance {
    locked: 0
    spendable: Balance {
        immature: 0
        confirmed: utxo2+utxo3
        trusted_pending: 0
        untrusted_pending: utxo1
     }
}

This looks fine, but when we introduce timelocks, things start to get weird...
get_spending_blan_balance(B) returns

SpendingPlanBalance {
    locked: utxo3+utxo1
    spendable: Balance {
        immature: 0
        confirmed: utxo2
        trusted_pending: 0
        untrusted_pending: 0
     }
}

In this case, we don't have the timelock as an asset, so we can spend just utxo3 (the timelock is already satisfied). I don't know about you, but I find this a bit confusing...

get_spending_blan_balance(B + older(timelock)) returns

SpendingPlanBalance {
    locked: 0
    spendable: Balance {
        immature: 0
        confirmed: utxo2+utxo3
        trusted_pending: 0
        untrusted_pending: utxo1
     }
}

This is weird as well: I'm saying that I'll wait older(timelock) - should utxo1 be untrusted_pending, then? In timelock blocks it will be confirmed and spendable, so maybe it should be confirmed: utxo1+utxo2+utxo3?

Let me know what you think :)

[notmandatory]

Topic came up to also the names "trusted_pending" and "untrusted_pending" aren't accurate. @danielabrozzoni may have some suggestions for other naming. My 2 sats is to not make a distinction and only have a "pending" balance 🙂.

See comment: https://github.com/bitcoindevkit/bdk/pull/976/files#r1210283701

[notmandatory]

I added this to 1.0.0-alpha.3 since it's a wallet breaking change so would be good to get it cleaned up now.

[notmandatory]

This is not urgent and can be done in a future major release so removing from 1.0 milestone.

[notmandatory]

Fixed by #640 .