Concurrent block processing #931

RCasatta · 2023-08-29T15:32:20Z

By changing the signature of the for_blocks function, avoiding the FnMut and returning data instead, we can process blocks in parallel.
This requires changing the code using the function because at the moment closures are using mutability in the context.

The status of the PR is very early, there are bugs sometimes with TODO, reviewers should avoid in-depth review at the moment, just want to demonstrate feasibility and have early feedback on the concept.

On my machine, with a node with an empty mempool the time taken by
./history.py 1FeexV6bAHb8ybZjqQMjJrcCrHGW9sb6uF --no-merkle-proofs
is reduced from about 3.5s to 2.5s

RCasatta · 2023-08-29T15:32:56Z

src/p2p.rs

            }
-            Ok(())
+            let x: Vec<_> = result.into_par_iter().map(|(a, b)| func(a, b)).collect(); // TODO restore observe


should instead start processing as soon as the data arrives, instead of starting when all the data arrives

RCasatta · 2023-08-30T15:45:59Z

This comment still applies but could be addressed in a subsequent PR.
Other known TODOs should be addressed

RCasatta · 2023-08-30T15:47:36Z

src/db.rs

+        if self.height < other.height {
+            self.tip_row = other.tip_row
+        }


This logic could bring to intermediate invalid states (with middle heights still missing).
However, since all batches are always merged should be okay at the end

src/tracker.rs

src/p2p.rs

RCasatta · 2023-10-07T07:31:20Z

Is something needed on my end?
I see CI failures but I am not sure, any suggestion is welcome

romanz · 2023-10-07T18:49:25Z

src/db.rs

+        self.spending_rows.extend(other.spending_rows.into_iter());
+        self.txid_rows.extend(other.txid_rows.into_iter());
+        if self.height < other.height {
+            self.tip_row = other.tip_row


Should we also update self.height?

I don't think so -> #931 (comment)

Which is not ideal at all, but I am not sure if there are other better options, or if I should keep this and add a comment

romanz · 2023-10-14T19:08:36Z

I see CI failures but I am not sure, any suggestion is welcome

IIUC, it seems to be related to this PR... could you please take a look?

romanz · 2023-11-20T19:46:37Z

src/p2p.rs

+                        let _ = send.send(r);
+                    });
+                }
+                let result: Result<Vec<_>, std::sync::mpsc::RecvError> =


Is it OK to block here (from rayon point of view)?
Asking, since when using this patch, it seems that a task is being spawned, but seems to not run:

[2023-11-20T19:44:54.785Z DEBUG electrs::p2p] peer inventory: [Block(0x423cfe9f541e1425b02b93d8db56f8fecdcb0ce8f58d33102d205c73f6bb037e)] [2023-11-20T19:44:54.786Z DEBUG electrs::p2p] got 1 new headers [2023-11-20T19:44:54.786Z INFO electrs::index] indexing 1 blocks: [111..111] [2023-11-20T19:44:54.786Z DEBUG electrs::p2p] loading 1 blocks [2023-11-20T19:44:54.786Z DEBUG electrs::p2p] spawning task for 423cfe9f541e1425b02b93d8db56f8fecdcb0ce8f58d33102d205c73f6bb037e [2023-11-20T19:44:54.786Z DEBUG electrs::p2p] waiting for 1 blocks [2023-11-20T19:44:54.786Z DEBUG electrs::p2p] processing 423cfe9f541e1425b02b93d8db56f8fecdcb0ce8f58d33102d205c73f6bb037e [2023-11-20T19:44:54.786Z DEBUG electrs::index] writing 3 funding and 11 spending rows from 2 transactions, 1 blocks [2023-11-20T19:44:54.843Z INFO electrs::signals] notified via SIG10 [2023-11-20T19:44:54.866Z INFO electrs::chain] chain updated: tip=423cfe9f541e1425b02b93d8db56f8fecdcb0ce8f58d33102d205c73f6bb037e, height=111 [2023-11-20T19:44:54.866Z DEBUG electrs::p2p] loading 1 blocks [2023-11-20T19:44:54.866Z DEBUG electrs::p2p] spawning task for 423cfe9f541e1425b02b93d8db56f8fecdcb0ce8f58d33102d205c73f6bb037e [2023-11-20T19:44:54.866Z DEBUG electrs::p2p] waiting for 1 blocks <it seems to get stuck here> [2023-11-20T19:45:48.880Z DEBUG electrs::server] 0: recv {"jsonrpc":"2.0","method":"blockchain.estimatefee","id":283,"params":[25]} [2023-11-20T19:45:48.880Z DEBUG electrs::server] 0: recv {"jsonrpc":"2.0","method":"blockchain.estimatefee","id":284,"params":[10]} [2023-11-20T19:45:48.880Z DEBUG electrs::server] 0: recv {"jsonrpc":"2.0","method":"blockchain.estimatefee","id":285,"params":[5]} [2023-11-20T19:45:48.880Z DEBUG electrs::server] 0: recv {"jsonrpc":"2.0","method":"blockchain.estimatefee","id":286,"params":[2]} [2023-11-20T19:45:48.889Z DEBUG electrs::server] 0: recv {"jsonrpc":"2.0","method":"mempool.get_fee_histogram","id":287}

Not sure, but it may be related to rayon-rs/rayon#835... WDYT?

It seems a good catch! Did you have a chance to try the patch with rayon::in_place_scope instead of rayon::scope as suggested in the issue? I'll try tomorrow anyway :)

Another alternative maybe could be to use Arc<Mutex<Vec<R>>> instead of the channel (so the blocking happens inside the spawn)

Unfortunately, I can't emulate the deadlock locally (I can see the output of another debug print after recv), maybe it depends on the number of threads used by rayon.
I am going to push a temporary commit to use the CI for this

Unfortunately, the CI is not happy with in_place_scope and neither with the Mutex version https://github.com/RCasatta/electrs/actions/runs/6941492681/job/18882464601

I can't emulate the deadlock locally (I can see the output of another debug print after recv)

With locally I mean without the docker...

How can I see the logs with the docker command as it is done (docker run -v $PWD/contrib/:/contrib -v $PWD/tests/:/tests --rm electrs:tests bash /tests/run.sh) in the CI ?

How can I see the logs with the docker command as it is done (docker run -v $PWD/contrib/:/contrib -v $PWD/tests/:/tests --rm electrs:tests bash /tests/run.sh) in the CI ?

electrs logs are stored under /data/electrs (inside the container):

electrs/tests/run.sh

Line 47 in a01d58c

2> data/electrs/regtest-debug.log &

Sorry for the dumb questions, but if the container dies after an error how to check inside the container?

Sorry, the logs are indeed unavailable when the container fails in CI...

I have added two more commits (8326249 & acaebd0), so it should be possible to reproduce the issue locally by:

$ docker build -f Dockerfile.ci . --rm -t electrs:tests-bug $ docker run -v $PWD/contrib/:/contrib -v /tmp/data:/data -v $PWD/tests/:/tests --rm electrs:tests-bug bash -x /tests/run.sh

The logs should be under /tmp/data/ after the container exits.

Thanks, in the meantime I launched a couple of jobs on the CI.
The bad news is that the in_place_scope seems to deadlock the same https://github.com/RCasatta/electrs/actions/runs/6947160675
The good news is that the Mutex version isn't deadlocking https://github.com/RCasatta/electrs/actions/runs/6946996655
But before cleaning up the Mutex version I think it could be possible a mutex-free solution...

But before cleaning up the Mutex version I think it could be possible a mutex-free solution...

I was considering to use a Vec<OnceLock<R>> for thread results, however OnceLock has MSRV 1.70
So, I changed the Mutex<Vec<R>> to Vec<Mutex<Option<R>>> so that there are no blocking call excluding the end of the rayon scope

requires changing the signature of the function from FnMut -> Fn and return data instead of relying on mutating the context

romanz · 2023-11-28T20:01:24Z

Thanks @RCasatta! Will review at the weekend :)

romanz · 2023-12-01T20:41:39Z

src/p2p.rs

-                })?;
-                self.blocks_duration
-                    .observe_duration("process", || func(hash, block));
+            let mut result = Vec::with_capacity(blockhashes_len);


If possible, can we use 5aba2a1 to simplify this method?

Sure! It is much better I didn't think about that

src/tracker.rs

romanz · 2023-12-02T17:25:31Z

src/status.rs

+            })?;
+        for (b, h) in inputs_filtering {
+            let e = result.entry(b).or_default();
+            e.extend(h);


It may happen that the same transaction both funds and spends the same scripthash - so we probably need to "merge" both funding and spending TxEntry instances -> f43de8f

LGTM, sorry to introduce so many bugs in the proposed PR, but it's pretty hard to remember all these cases, ideally some testing should enforce these...

No problem, I definitely should add more tests :)

RCasatta commented Aug 29, 2023

View reviewed changes

romanz added the performance label Aug 29, 2023

romanz self-assigned this Aug 29, 2023

RCasatta force-pushed the par_for_blocks branch from 73eae57 to b2fcb27 Compare August 30, 2023 15:21

RCasatta commented Aug 30, 2023

View reviewed changes

RCasatta marked this pull request as ready for review September 8, 2023 09:08

RCasatta force-pushed the par_for_blocks branch from b2fcb27 to 93e2161 Compare September 9, 2023 19:11

RCasatta commented Sep 9, 2023

View reviewed changes

src/tracker.rs Show resolved Hide resolved

romanz reviewed Sep 16, 2023

View reviewed changes

src/p2p.rs Outdated Show resolved Hide resolved

romanz reviewed Oct 7, 2023

View reviewed changes

romanz reviewed Nov 20, 2023

View reviewed changes

RCasatta added 2 commits November 21, 2023 18:46

for_blocks apply closure in parallel

5829edd

requires changing the signature of the function from FnMut -> Fn and return data instead of relying on mutating the context

scope and spawn instead of par iter

f7c8cca

RCasatta force-pushed the par_for_blocks branch from 7e693de to f7c8cca Compare November 21, 2023 19:48

RCasatta requested a review from romanz November 28, 2023 08:45

romanz reviewed Dec 1, 2023

View reviewed changes

romanz force-pushed the par_for_blocks branch 2 times, most recently from 18a31e3 to f7c8cca Compare December 2, 2023 10:25

romanz reviewed Dec 2, 2023

View reviewed changes

romanz mentioned this pull request Jan 6, 2024

Use bitcoind crate to add more integration tests #988

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concurrent block processing #931

Concurrent block processing #931

RCasatta commented Aug 29, 2023

RCasatta Aug 29, 2023

RCasatta commented Aug 30, 2023

RCasatta Aug 30, 2023

RCasatta commented Oct 7, 2023

romanz Oct 7, 2023

RCasatta Oct 13, 2023 •

edited

Loading

romanz commented Oct 14, 2023

romanz Nov 20, 2023

RCasatta Nov 20, 2023 •

edited

Loading

RCasatta Nov 21, 2023

RCasatta Nov 21, 2023 •

edited

Loading

RCasatta Nov 21, 2023

romanz Nov 21, 2023

RCasatta Nov 21, 2023

romanz Nov 21, 2023

RCasatta Nov 21, 2023

RCasatta Nov 21, 2023

romanz commented Nov 28, 2023

romanz Dec 1, 2023

RCasatta Dec 1, 2023

romanz Dec 2, 2023

RCasatta Dec 4, 2023

romanz Dec 4, 2023

Concurrent block processing #931

Are you sure you want to change the base?

Concurrent block processing #931

Conversation

RCasatta commented Aug 29, 2023

Choose a reason for hiding this comment

RCasatta commented Aug 30, 2023

Choose a reason for hiding this comment

RCasatta commented Oct 7, 2023

Choose a reason for hiding this comment

RCasatta Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

romanz commented Oct 14, 2023

Choose a reason for hiding this comment

RCasatta Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCasatta Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanz commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCasatta Oct 13, 2023 •

edited

Loading

RCasatta Nov 20, 2023 •

edited

Loading

RCasatta Nov 21, 2023 •

edited

Loading