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 disconnect-block functionality for LocalChain and Wallet #1271

Open
evanlinjin opened this issue Jan 13, 2024 · 4 comments · Fixed by #1276
Open

Add disconnect-block functionality for LocalChain and Wallet #1271

evanlinjin opened this issue Jan 13, 2024 · 4 comments · Fixed by #1276
Assignees
Labels
discussion There's still a discussion ongoing module-blockchain new feature New feature or request

Comments

@evanlinjin
Copy link
Member

Describe the enhancement

impl LocalChain {
    /// Removes blocks from the given `block_id`.
    ///
    /// Will remove blocks with a height equal or greater than `block_id`,
    /// but only if `block_id` exists in the chain.
    pub fn disconnect_block(&mut self, block_id: BlockId) -> ChangeSet {
        todo!()
    }
}

Use case

The APIs of utreexod (based on btcd) and bitcoind ZeroMQ has disconnect-block notifications. BDK should be able to handle such notifications.

@evanlinjin evanlinjin added the new feature New feature or request label Jan 13, 2024
@evanlinjin evanlinjin added this to the 1.0.0 milestone Jan 13, 2024
@evanlinjin evanlinjin added this to BDK Jan 13, 2024
@evanlinjin evanlinjin self-assigned this Jan 15, 2024
@github-project-automation github-project-automation bot moved this to Done in BDK Jan 15, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Jan 17, 2024

post fix concept NACK. I interpret these messages are informational (something to log out). You just need to wait for the block that replaces it which will automatically evict the block. Eagerly evicting blocks means you end up in a state where your wallet says it has less money than it actually does. This could be unexpected and harmful -- imagine you have a emergency notification that goes out to the whole company if there is a sudden drop in Bitcoin holdings. If you just wait for the new block any funds you received in the evicted block are likely to appear in the new one so you never end up in this state.

Is there some motivation for reacting to these messages that I'm missing?

@LLFourn LLFourn reopened this Jan 17, 2024
@notmandatory notmandatory moved this from Done to Needs Review in BDK Jan 18, 2024
@nondiremanuel nondiremanuel moved this from Needs Review to Todo in BDK Jan 25, 2024
@nondiremanuel nondiremanuel added the discussion There's still a discussion ongoing label Jan 25, 2024
@nondiremanuel nondiremanuel moved this from Todo to Discussion in BDK Jan 25, 2024
@notmandatory
Copy link
Member

looks like this was fixed with #1276.

@github-project-automation github-project-automation bot moved this from Discussion to Done in BDK Mar 18, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Mar 18, 2024

I'm guessing you didn't see my comment above where I question whether we should actually do this. If you think I should make a new issue let me know.

@LLFourn LLFourn reopened this Mar 18, 2024
@notmandatory
Copy link
Member

@LLFourn sorry about that, no need to create a new issue. I just saw the closed related issue and didn't see your first reopen for this one.

@evanlinjin based on Lloyd's reasoning should we revert #1276 or is there some particular use case for this new function, such as for utrexo that can't wait for a new block to evict reorg'd out blocks?

If there is a reason to keep this function that everyone agrees on it sounds like we should at least add a strong warning in the docs that it could cause inaccurate utxo balances if misused.

@nondiremanuel nondiremanuel moved this from Done to Discussion in BDK Mar 21, 2024
@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing module-blockchain new feature New feature or request
Projects
Status: Discussion
Development

Successfully merging a pull request may close this issue.

4 participants