search

Blockstream / electrs

An efficient re-implementation of Electrum Server in Rust
MIT License
324 stars 131 forks source link

Split up mempool fetch and mempool write #77

Closed jamesdorfman closed 7 months ago

jamesdorfman commented 8 months ago

Currently, the mempool's lock is acquired before doing the following steps to update the in-memory version of the mempool:

  1. Fetch the old mempool's TXIDs from disk
  2. Fetch the new mempool's TXIDs over RPC
  3. Fetch the full transactions of the new TXIDs
  4. Write the new transactions to the in-memory transaction index

Right now, all four steps are done while holding a write lock on the mempool. We suspect that this is causing incoming requests which read from the query's mempool to fail, because steps 1 - 4 together

In reality, step 1 only needs a read lock on the mempool, step 2 and 3 need no lock and only step 4 needs the write lock. This PR updates the code to only acquire a write lock for step 4.

It also adds logging around each step, so that we can more-easily see were the bottlenecks are when long response times occasionally occur.

Since the lock is dropped between step 1 and step 4, this opens up potential consistency issues. However, step 4 is the only thing which updates the mempool -- no updates come from other threads, so this shouldn't cause any issues.

jamesdorfman commented 8 months ago

@shesek Thanks for the review! I believe I've addressed your latest comments. Please let me know if I missed anything / misunderstood.

junderw commented 7 months ago

See this commit on the PR based off this PR: https://github.com/mempool/electrs/pull/87/commits/9e0ecad4d4f07c10dad35b91d4aa5b8355052e34

If someone tried to call broadcast with a tx we are trying to update in between the grabbing of old_txids and before adding the tx in the update call, the tx will already exist and since the history entries are in Vecs you could get duplicate history entries.