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

Chain interface: drying up and splitting it in multiple ones #17668

Closed
ariard opened this issue Dec 4, 2019 · 4 comments
Closed

Chain interface: drying up and splitting it in multiple ones #17668

ariard opened this issue Dec 4, 2019 · 4 comments

Comments

@ariard
Copy link
Member

ariard commented Dec 4, 2019

Once #16426 merged, we should keep drying up the interface to make it fully asynchronously, few ideas to brainstorm on:

  • all getHeight, getBlockHeight and findFirstBlockWithTimeAndHeight-style methods should be removed once rescan logic moved in its own thread behind the interface (I have already a functional branch for this)
  • all mempool related methods (mempoolMinFee) could be moved in its own Mempool::interface, client could care only about fee estimation, make it asynchronous too
  • initMessage, initError and others could be moved in Node::interface
  • handleRpc and others, wallet should have its own RPC server

I think last two issues could be started today, they don't rely on any other PRs.

We may also have a Broadcast interface, tracking the chain state and broadcasting a tx are really 2 different logics.

@ryanofsky pretty sure you have thoughts on it too.

@ryanofsky
Copy link
Contributor

Things suggested above sound good to me. There's a TODO comment with similar suggestions.

  • initMessage, initError and others could be moved in Node::interface

The interfaces::Node class is intended more for the gui and would give the wallet more functionality than it needs but I've previously thought these could go in a small interfaces::Ui or interfaces::WalletClient class along with the loadWallet notification (#15288 (comment))

@maflcko
Copy link
Member

maflcko commented Aug 3, 2022

Is this still an issue?

@ariard
Copy link
Member Author

ariard commented Aug 3, 2022

@MarcoFalke

From a quick read, I would say the suggestion about initMessage and initError is still relevant against today's code (a). The other one about isolating the rescan logic on the wallet-side is still relevant (b). About a "Mempool::interface" I think some information could be relevant in the recent discussions about mempool-code reorganizations (c). Wallet should have its own RPC server is already documented as a TODO in doc/multiprocess.md iirc (d). A new Broadcast or extended Chain interface would be relevant for AltNet (e).

I don't have interest in doing a) or b) or d) in the coming future. I do have strong interest in c) and e) soon.

From an issue-management standpoint, feel free to close the issue or re-dispatch the relevant informations elsewhere.

@ryanofsky
Copy link
Contributor

I think a lot of what's discussed here is still relevant, but there's not really a need to keep the issue open. I think any new ideas that we don't want to forget about could be added to the TODO comment. Otherwise I don't think there's any decisions that need to be discussed or ongoing development that needs to be tracked. The chain interface is already a lot simpler than it used to be after #16426, and it could just be broken up a little more.

@bitcoin bitcoin locked and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants