fix: Fix BTCRelay Chain Reorganization to Prioritize Work Over Length

[nakul1010]

Closes #1186.

Code Flow based on discussion in comments

1. Migrate from v0 to v1 using the migrate_from_v0_to_v1 function:

   • This function inserts the chainwork for the latest bitcoin block.

2. Keep updating the chainwork for historical blocks using the set_chainwork_for_block function:

   • The store_block_header extension will take over once it syncs to the latest block, as it will also attempt to update the chainwork.
   • Changes on the client side are required for calling the extrinsic set_chainwork_for_block for all the historical bitcoin blocks.
   • Retire/Pause the set_chainwork_for_block extension after a successful sync.

3. The ReOrg will switch from the longest chain to the most work when the chainwork is found for the main chain block as well as the latest block header submitted.

ToDos

• Add required methods to switch from the longest chain to the most work
• Migration from v0 to v1
• Unit Test Cases
• Benchmarking

[nakul1010]

The PR 1149 should be merged before merging this PR.

[nakul1010]

@gregdhill for test case store_block_header_simple_fork_succeeds

Problem 1:
When attempting to retrieve the chainwork for the given blockchain:

BlockChain { chain_id: 1, start_height: 1, max_height: 2 }

It emits an error MissingBlockHeight. This error arises because it's trying to obtain the block hash for chain_id: 1, block_height: 0. The problem lies in the design of the storage (ChainHashes), where block 0 is stored with the key chain_id 0 instead of 1. In the Electrum codebase here, the get_hash function only accepts the height parameter and doesn't consider chain_id.

Problem 2:
The same test case will encounter issues when calculating work_in_single_header since the block hash for cached_height 2016 won't be available.

---

Is the solution to get block_hash irrespective of chain_id by just providing the key as block_height or am i missing out something?

[gregdhill]

• Problem 1: You can iterate over the previous forks, electrum does that here
• Problem 2: You'll have to figure out a good way to test this :) maybe you can mock it or provide some dummy value in the ChainWork storage map

[sander2]

I'm not sure the current approach is the best, it does a lot of iterating.. Fwiw I initially proposed an iteration-free solution, I think maybe we should consider it @gregdhill :

• add a new map of blockhash -> chainwork
• add a root-only function, or a function called from a runtime migration, that sets the chainwork of some recent block
• add a temporary non-root function that takes a blockheader x, and if x.prev has an associated chainwork, set the chainwork of x. This function will be used to catch up to to the bitcoin chain
• in the relay-function: when relaying a block x, if x.prev has associated chainwork, store the new chainwork of x.
• add a root-only function that changes chain selection from longest-chain to most-work. We only call this once the chainhead has associated chainwork set

[sander2]

I'm not sure it's a good idea to use the electrum function unchanged. In fact, I don't think it'll work, because

• (1) it's super expensive - it iterates all the way from block zero. We might exceed the block time because of all the iteration
• (2) We didn't initialize the relay at height 0. The chainwork_of_header_at_height function would fail.

This is a pretty high-risk change, and I would ask you to test it thoroughly. If I'm right and this doesn't work, it could've been caught with a chopsticks test.

[nakul1010]

yes, reviewing the code back again even if the loop over the target it would still take btc_best_block/2016 ie 808,849 /2016 = 402 iterations. Ideally, the code should have been added from parachain genesis block to avoid iterations. I will still do the chopsticks test and put the results in another comment.

[nakul1010]

add a temporary non-root function that takes a blockheader x, and if x.prev has an associated chainwork, set the chainwork of x. This function will be used to catch up to to the bitcoin chain

@sander2 This approach sounds good. I have a question about the non-root function mentioned in the comment:

• I expect the work done formula to remain the same as here, but then the target is a U256 value, and converting it to u32 doesn't make sense, as explained in this fixme. So, can the new mapping be blockhash(H256Le) -> chainwork(U512) type instead of blockhash(H256Le) -> chainwork(u32) which is currently being coded out?

[nakul1010]

Based on the above comments was found that the electrum/summa relay code cannot be imported directly.

New Code Flow:

1. Migrate from v0 to v1 using the migrate_from_v0_to_v1 function:

   • This function inserts the chainwork for the latest block.

2. Keep updating the chainwork for historical blocks using the set_chainwork_for_block function:

   • The store_block_header extension will take over once it syncs to the latest block, as it will also attempt to update the chainwork. However, if it fails, it will not return an error.
   • Changes on the client side are required for calling the extrinsic set_chainwork_for_block for all the historical bitcoin blocks.
   • Retire/Pause the set_chainwork_for_block extension after a successful sync.

3. Call the set_most_work_parameter function through governance:

   • This will make a switch from the longest chain to the chain with the most work.
   • Retire/Pause the set_most_work_parameter extension after a successful sync.

Limitations:

• Switching from the longest chain to the chain with the most work using set_most_work_parameter is slightly tricky.

  • Since it is a governance call and will be executed in a future block 'n' after migration has been applied to block 'm', Bitcoin blocks mined between 'm' to 'n' para chain blocks need to have chainwork. After executing set_most_work_parameter, the longest chain reorg will be completely skipped, and it will work according to the flow mentioned above, as the store_block_header also attempts to apply chainwork. However, in the case of failure to set chainwork for previous blocks by the client, a safety net should be ideal. This would require adding one more storage to store the latest block, which can be used by the set_most_work_parameter extension, as the parameter can only be set if the latest Bitcoin block has chainwork. What are your thoughts on this @gregdhill and @sander2.

• Stored ChainWork as U512 instead of u32 required based on mathematical value 2^256 needed during calculation here.

[sander2]

Based on the above comments was found that the electrum/summa relay code cannot be imported directly.

New Code Flow:

1. Migrate from v0 to v1 using the `migrate_from_v0_to_v1` function:
   
   * This function inserts the chainwork for the latest block.

2. Keep updating the chainwork for historical blocks using the `set_chainwork_for_block` function:
   
   * The `store_block_header` extension will take over once it syncs to the latest block, as it will also attempt to update the chainwork. However, if it fails, it will not return an error.
   * Changes on the client side are required for calling the extrinsic `set_chainwork_for_block` for all the historical bitcoin blocks.
   * Retire/Pause the `set_chainwork_for_block` extension after a successful sync.

3. Call the `set_most_work_parameter` function through governance:
   
   * This will make a switch from the longest chain to the chain with the most work.
   * Retire/Pause the `set_most_work_parameter` extension after a successful sync.

Limitations:

* Switching from the longest chain to the chain with the most work using `set_most_work_parameter` is slightly tricky.
  
  * Since it is a governance call and will be executed in a future block 'n' after migration has been applied to block 'm', Bitcoin blocks mined between 'm' to 'n' para chain blocks need to have chainwork. After executing `set_most_work_parameter`, the longest chain reorg will be completely skipped, and it will work according to the flow mentioned above, as the `store_block_header` also attempts to apply chainwork. However, in the case of failure to set chainwork for previous blocks by the client, a safety net should be ideal. This would require adding one more storage to store the latest block, which can be used by the `set_most_work_parameter` extension, as the parameter can only be set if the latest Bitcoin block has chainwork. What are your thoughts on this @gregdhill and @sander2.

I think we can do away with set_most_work_parameter and ReOrgAccordingToChainWork. Instead, we can automatically switch over when the mainchain has a known chainwork. The only edge case I can see is if there's an on-going fork at the moment we submit the latest chainwork but that's pretty unlikely so idk if we have to account for that.

* Stored ChainWork as `U512` instead of `u32` required based on mathematical value `2^256` needed during calculation [here](https://github.com/spesmilo/electrum/blob/cee22abcb5544c5a6fa7f8a8108ccda9c32c2e29/electrum/blockchain.py#L586C1-L587C1).

That shouldn't be required, since the formula is work = ((2 ** 256 - target - 1) // (target + 1)) + 1, which is the same as rust's (U256::MAX - target - 1) / (target + 1) + 1 which fits in U256 just fine

[sander2]

nit: I personally don't like getter/setter functions around storage variables if it literally on a wrapper around the get/insert since when reading the code, you have to check the function to see if any logic is hidden there.

[nakul1010]

noted

[nakul1010]

This adds a headache since different hash and chainwork need to be provided when updating the testnet and mainnet. Is there any smart way to do this?
Require for doing chopsticks migration test as well

[nakul1010]

the if condition is required as after migration there will be bitcoin blocks that will not have a chainwork, the relayer would require some block time to successfully sync the chainwork for the latest BTC block.