LLFourn / bdk_core_staging

Staging area for bdk_core initial development
15 stars 12 forks source link

"ChangeSet" API is error prone in multi-threaded contexts #161

Open LLFourn opened 1 year ago

LLFourn commented 1 year ago

Non-monotone changes (i.e. SparseChain changesets) must be generated and applied in order. If in between generating a changeset and applying it you apply another changeset that changeset must be thrown away. This is especially likely to happen if you need to do some IO after generating a changeset before you apply it and so you drop your mutex lock on the chain allowing another thread to make modifications while you wait for the result of your IO.

We current have baked in this mistake into our electrum example and the idea the "inflate_changeset" API on chaingraph. There we first generate a sparse chain changeset, we do some IO to get the full transactions (and drop the lock), and then inflate the changeset and apply it. What if another thread makes contradictory changes to the blockchain during this time i.e. there is a new block and the blocks and transactions have moved position since then? It breaks. This particular approach can be changed by instead of keeping a changeset over the IO boundry we inflate to a full ChainGraph and apply that -- this will enforce consistency.

We may wan to come up with a more general solution that makes it impossible to make this mistake:

  1. 70 would do this but we could also,

  2. Do not allow applying changesets API but rather stage them inside the objects themselves and commit them when appropriate to return the changeset which can be persisted.

This does not need to be fixed for first release.

evanlinjin commented 1 year ago

Another option is to:

  1. Use sparse_chain::ChangeSets with ChainGraph (instead of another ChangeSet type for chain_graph).
  2. Add ability to add auxiliary transactions directly into TxGraph of ChainGraph.

The API will be used like this:

  1. ChainGraph::apply_changeset() will error if changeset has missing full transactions.
  2. We can poll request for the missing txs from electrum server.
  3. Add the missing transactions into ChainGraph without ChainPosition.
  4. ChainGraph::apply_changest() again.