Adds --forceupdate to teosd #216

sr-gi · 2023-05-04T04:12:49Z

If a tower hasn't been running for a long time and the backend runs in pruned mode it could be the case that by the time the tower comes back online, the Last known block by the tower is not being known by the node anymore. In this situation, the tower cannot bootstrap normally, given the cache cannot be populated.

This commits adds a new argument to teosd (--forceupdate) that can be used to force a tower to update its last known block to the earliest known block by the backend under this situation. Notice that doing so may make the tower miss some of its state transitions (the ones triggered by missed blocks), so this must be done as a last resource.

Fixes #209
Supersedes #212

Testing

This can be tested easily in regtest by running bitcoind with the (hidden, undocumented) -fastprune modifier. That would make blocks smaller and easier to prune. After that, you can prune the chain using bitcoin-cli pruneblockchain x where x is your target prune height. The pruning is not likely to happen to the exact block you specified, given block files are pruned as a whole, so the chain would be pruned to the closest value that allows a bock file to be deleted.

So the flow of testing should be:

Start teosd and mine a few blocks so there is a last_known_block known by the tower
Shut down the tower
Mine a few thousand more blocks
Call bitcoin-cli pruneblockchain x and checked the returned y value
If y - IRREVOCABLY_RESOLVED < last_known_block_height, continue, otherwise go back to 3.
Run teosd again and test it with and without --forceupdate

sr-gi · 2023-05-04T04:18:46Z

I've tested this manually and it seems to run just fine.

In order to test this you'd need a tower that has a last known block at height n and a node with a chain that has pruned that same block. I haven't been able to test this on regtest given pruning is based on how much block data we are holding (not how many blocks) and regtest blocks are mainly empty, so the threshold is never met. However, I've been able to test this using mainnet and an old tower state.

I may add some E2E testing for this if I manage to create a test where full blocks are created, so eviction can be triggered.

mariocynicys

I'm confused about what the chain_monitor would do given it's being passed an old/pruned tip. Is poll_best_tip() gonna just skip the pruned blocks between tip and the best tip?

mariocynicys · 2023-05-04T09:31:13Z

teos/src/main.rs

            .validate(block_hash)
            .unwrap()


Ummm, doesn't this fail as well when we are out of blocks?

Actually, it does not because the headers chain is preserved even if the blocks are pruned.

mariocynicys · 2023-05-04T09:31:39Z

teos/src/main.rs

+                    bitcoin_cli
+                        .deref()
+                        .get_header(&hash, Some(height as u32))
+                        .await
+                        .unwrap()
+                        .validate(hash)
+                        .unwrap(),


nit: can we move this into a variable and call it tip.

mariocynicys · 2023-05-04T09:35:21Z

teos/src/main.rs

+        .await
+    {
+        Ok(blocks) => blocks,
+        Err(_) => {


If we go into this branch we would return some last_n_blocks whose tip isn't the same as the tip variable we are using in the initialization of the tower components (watcher, etc...).

mariocynicys · 2023-05-04T09:41:18Z

teos/src/main.rs

+                let hash = rpc
+                    .get_block_hash(height + IRREVOCABLY_RESOLVED as u64)
+                    .unwrap();
+                if let Ok(blocks) = get_last_n_blocks(


These n blocks won't be delivered to the tower components, right?

sr-gi · 2023-05-04T22:42:14Z

I'm confused about what the chain_monitor would do given it's being passed an old/pruned tip. Is poll_best_tip() gonna just skip the pruned blocks between tip and the best tip?

Yeah, you're right. I've come up with a better solution than this, will force push asap

sr-gi · 2023-05-05T03:23:37Z

This should make more sense now

mariocynicys · 2023-05-05T05:35:22Z

teos/src/main.rs

+        // If we are running in pruned mode some data may be missing (if we happen to have been offline for a wile)
+        if let Some(prune_height) = rpc.get_blockchain_info().unwrap().prune_height {
+            if last_known_header.height < prune_height as u32 {
+                log::debug!(


I think this should be logged as a warning.

mariocynicys · 2023-05-05T05:55:49Z

teos/src/main.rs

+
+        // If we are running in pruned mode some data may be missing (if we happen to have been offline for a wile)
+        if let Some(prune_height) = rpc.get_blockchain_info().unwrap().prune_height {
+            if last_known_header.height < prune_height as u32 {


We would still fail if some of the 100 blocks that proceeds the last known one are pruned.

The check should be: last_known_header.height - 100 < prune_height as u32

<=, but you're right

Oh, NVM, I was thinking about the positive case (>=)

sr-gi · 2023-05-05T15:12:18Z

I fixed suggestions and updated some of the logs since I didn't like them much.

I've also updated the PR description with guidelines for how to test this in regtest.

mariocynicys

Some nits, looks good otherwise.
Didn't manually test it yet though.

teos/src/main.rs

sr-gi · 2023-05-06T14:13:36Z

Nits should be fixed. I'd be nice to get a tACK

mariocynicys · 2023-05-06T17:53:06Z

Nits should be fixed. I'd be nice to get a tACK

Yeah will work on that tonight.

mariocynicys

tACK

Some comments (again :p) inline, looks good otherwise.

mariocynicys · 2023-05-06T18:25:30Z

teos/src/config.rs

@@ -102,6 +102,11 @@ pub struct Opt {
    #[structopt(long)]
    pub tor_support: bool,

+    /// Forces the tower to run even if the underlying chain has gone too far out of sync. This can only happen
+    // if the node is being run in pruned mode.


The second line being a non-doc comment makes structopt ignore it in the help message.

mariocynicys · 2023-05-06T19:01:14Z

teos/src/main.rs

+        if let Some(prune_height) = rpc.get_blockchain_info().unwrap().prune_height {
+            if last_known_header.height - IRREVOCABLY_RESOLVED < prune_height as u32 {
+                log::warn!(
+                    "Cannot load blocks in the range {}-{}. Chain has gone to far out of sync",


typo: "totoo far our of sync"

mariocynicys · 2023-05-06T19:19:10Z

teos/src/main.rs

+
+        // If we are running in pruned mode some data may be missing (if we happen to have been offline for a while)
+        if let Some(prune_height) = rpc.get_blockchain_info().unwrap().prune_height {
+            if last_known_header.height - IRREVOCABLY_RESOLVED < prune_height as u32 {


This has an off-by-one error 😂, should be last_known_header.height - IRREVOCABLY_RESOLVED + 1 < prune_height as u32.

Let's say we last witnessed block 3000, then we will want to load blocks 2901-3000 (100 blocks). prune_height indicates the lowest block we have stored, so if prune_height is 2901 we should be fine, but last_known_header.height - IRREVOCABLY_RESOLVED will be 2900, which would make the check fail.

You're right

mariocynicys · 2023-05-06T19:19:40Z

teos/src/main.rs

+            if last_known_header.height - IRREVOCABLY_RESOLVED < prune_height as u32 {
+                log::warn!(
+                    "Cannot load blocks in the range {}-{}. Chain has gone to far out of sync",
+                    last_known_header.height - IRREVOCABLY_RESOLVED,


This should be last_known_header.height - IRREVOCABLY_RESOLVED + 1.

sr-gi · 2023-05-07T20:15:30Z

Addressed comments

mariocynicys

tACK c7f9f0a

If a tower hasn't been running for a long time and the backend runs in pruned mode it could be the case that by the time the tower comes back online, the Last known block by the tower is not being known by the node anymore. In this situation, the tower cannot bootstrap normally, given the cache cannot be populated. This commits adds a new argument to teosd (`--forceupdate`) that can be used to force a tower to update its last known block to the earliest known block by the backend under this situation. Notice that doing so may make the tower miss some of its state transitions (the ones triggered by missed blocks), so this must be done as a last resource.

sr-gi added the Seeking Code Review review me pls label May 4, 2023

sr-gi force-pushed the outdated_tower branch from a338453 to 5d78992 Compare May 4, 2023 04:22

mariocynicys reviewed May 4, 2023

View reviewed changes

sr-gi force-pushed the outdated_tower branch from 5d78992 to 9a0e914 Compare May 5, 2023 03:23

mariocynicys requested changes May 5, 2023

View reviewed changes

sr-gi force-pushed the outdated_tower branch from 9a0e914 to b104d94 Compare May 5, 2023 13:30

sr-gi mentioned this pull request May 5, 2023

Added Bypass flag so teosd work in pruned mode #212

Closed

mariocynicys reviewed May 6, 2023

View reviewed changes

teos/src/main.rs Outdated Show resolved Hide resolved

teos/src/main.rs Outdated Show resolved Hide resolved

teos/src/main.rs Outdated Show resolved Hide resolved

sr-gi force-pushed the outdated_tower branch from b104d94 to d52fdcb Compare May 6, 2023 14:13

mariocynicys requested changes May 6, 2023

View reviewed changes

mariocynicys mentioned this pull request May 7, 2023

Disallow dangerous parameters in the config file #217

Open

sr-gi force-pushed the outdated_tower branch from d52fdcb to c7f9f0a Compare May 7, 2023 20:15

mariocynicys approved these changes May 9, 2023

View reviewed changes

sr-gi removed the Seeking Code Review review me pls label May 9, 2023

sr-gi force-pushed the outdated_tower branch from c7f9f0a to 2bf555c Compare May 9, 2023 13:49

sr-gi merged commit 1a89c5d into talaia-labs:master May 9, 2023
6 checks passed

mariocynicys mentioned this pull request Jun 27, 2023

Edge case where outdated users will persist in the DB #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds --forceupdate to teosd #216

Adds --forceupdate to teosd #216

sr-gi commented May 4, 2023 •

edited

Loading

sr-gi commented May 4, 2023

mariocynicys left a comment

mariocynicys May 4, 2023

sr-gi May 4, 2023

mariocynicys May 4, 2023

mariocynicys May 4, 2023

mariocynicys May 4, 2023

sr-gi commented May 4, 2023

sr-gi commented May 5, 2023

mariocynicys May 5, 2023

mariocynicys May 5, 2023

sr-gi May 5, 2023

sr-gi May 5, 2023

sr-gi commented May 5, 2023

mariocynicys left a comment

sr-gi commented May 6, 2023

mariocynicys commented May 6, 2023 •

edited

Loading

mariocynicys left a comment

mariocynicys May 6, 2023

mariocynicys May 6, 2023

mariocynicys May 6, 2023

sr-gi May 7, 2023

mariocynicys May 6, 2023

sr-gi commented May 7, 2023

mariocynicys left a comment

Adds --forceupdate to teosd #216

Adds --forceupdate to teosd #216

Conversation

sr-gi commented May 4, 2023 • edited Loading

Testing

sr-gi commented May 4, 2023

mariocynicys left a 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

sr-gi commented May 4, 2023

sr-gi commented May 5, 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

sr-gi commented May 5, 2023

mariocynicys left a comment

Choose a reason for hiding this comment

sr-gi commented May 6, 2023

mariocynicys commented May 6, 2023 • edited Loading

mariocynicys left a 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

sr-gi commented May 7, 2023

mariocynicys left a comment

Choose a reason for hiding this comment

sr-gi commented May 4, 2023 •

edited

Loading

mariocynicys commented May 6, 2023 •

edited

Loading