Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

detect and resolve chain reorganization #1728

Merged
merged 55 commits into from
May 11, 2023
Merged

detect and resolve chain reorganization #1728

merged 55 commits into from
May 11, 2023

Conversation

borngraced
Copy link
Member

Verify the parent hash of each block to confirm its consistency with the hash of the preceding block. If it does not match, proceed to backtrack until a match is found. Then, proceed with indexing from the block height of the matched block.

fixes task 7 in #1612

Signed-off-by: borngraced <samuelonoja970@gmail.com>
+ unit test for electrum and wasm

Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced added the in progress Changes will be made from the author label Mar 22, 2023
@borngraced borngraced self-assigned this Mar 22, 2023
@borngraced borngraced changed the title check and fix for chain reorgorganization check and fix chain reorganization Mar 22, 2023
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced changed the title check and fix chain reorganization check and resolve chain reorganization Mar 22, 2023
@borngraced borngraced changed the title check and resolve chain reorganization detect and resolve chain reorganization Mar 22, 2023
@borngraced borngraced added under review and removed in progress Changes will be made from the author labels Mar 22, 2023
@borngraced borngraced added in progress Changes will be made from the author and removed under review labels Mar 22, 2023
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced added under review and removed in progress Changes will be made from the author labels Mar 23, 2023
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@shamardy
Copy link
Collaborator

@borngraced can you please fix CI errors? please also merge with latest dev for CI changes :)

Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced
Copy link
Member Author

ready now @shamardy

@shamardy shamardy removed the in progress Changes will be made from the author label May 5, 2023
@shamardy
Copy link
Collaborator

shamardy commented May 5, 2023

@borngraced can you please check my latest changes? You are the reviewer now 😅

@shamardy shamardy assigned borngraced and unassigned borngraced May 5, 2023
@shamardy shamardy requested a review from onur-ozkan May 5, 2023 23:24
Copy link
Member Author

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review check without testing the code
I'll do another review after testing the code

mm2src/coins/utxo/rpc_clients.rs Show resolved Hide resolved
mm2src/coins/utxo/rpc_clients.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_tests.rs Outdated Show resolved Hide resolved
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the enhancement!

Few notes from my side:

@onur-ozkan
Copy link
Member

LGTM once CI is green(except the common fails)

@shamardy
Copy link
Collaborator

shamardy commented May 9, 2023

@borngraced this PR is ready for next review from you.

Copy link
Member Author

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try testing the code to see if headers will be downloaded and stored as supposed but unfortunately they're not. I was getting Permanent error: Unknown destination address on my console.

Permanent Error: rpc_clients:2113] JsonRpcError { client_info: "coin: RICK", request: 
JsonRpcRequest { jsonrpc: "2.0", id: "34", method: "blockchain.block.headers", params: 
[Number(2), Number(16)] }, error: Internal("Unknown destination address 
electrum3.cipig.net:10017") }, on retrieving headers from server electrum3.cipig.net:10017

@shamardy
Copy link
Collaborator

I try testing the code to see if headers will be downloaded and stored as supposed but unfortunately they're not. I was getting Permanent error: Unknown destination address on my console.

Permanent Error: rpc_clients:2113] JsonRpcError { client_info: "coin: RICK", request: 
JsonRpcRequest { jsonrpc: "2.0", id: "34", method: "blockchain.block.headers", params: 
[Number(2), Number(16)] }, error: Internal("Unknown destination address 
electrum3.cipig.net:10017") }, on retrieving headers from server electrum3.cipig.net:10017

@borngraced I guess this was during coin activation not after since it tries to get blocks 2-16, you should have checked task::enable_utxo::status to know that the coin wasn't activated and returned this error which is the desired approach, since if there was a problem with one of the electrum servers on activation, this error indicates that the user shouldn't use this electrum server. Please remove electrum3.cipig.net:10017 from the electrum list and try again, please also provide me with the logs that led to that problem without removing electrum3.cipig.net:10017 to make sure that the problem was from the server, in the logs there should be a log that indicates why the url was removed from the connections list.

@cipig
Copy link
Member

cipig commented May 10, 2023

RICK electrums are fine
image
but keep in mind that every electrum server is restarted once a week, so you will loose connection if you do anything during the restart

@shamardy
Copy link
Collaborator

shamardy commented May 10, 2023

RICK electrums are fine
but keep in mind that every electrum server is restarted once a week, so you will loose connection if you do anything during the restart

The problem is if the connection was lost during activation only, if it was while mm2 is running, mm2 shouldn't remove the server and will use the other servers until this server is back. I also remember that you mentioned this restart to me while working on an old issue more than a year ago, I remember it was in tuesday if I am not mistaken, so maybe that's what happened when @borngraced was testing.

@cipig
Copy link
Member

cipig commented May 10, 2023

I also remember that you mentioned this restart to me while working on an old issue more than a year ago, I remember it was in tuesday if I am not mistaken

They are restarted at different times

50 9 * * 2 electrum1
50 17 * * 4 electrum2
50 1 * * 6 electrum3

the first 2 numbers are minute and hour... the last number is the weekday... 2, 4 and 6... "day of week 0–7 (0 or 7 is Sun, or use names)"... so electrum1 is Tuesday

@borngraced
Copy link
Member Author

borngraced commented May 10, 2023

I try testing the code to see if headers will be downloaded and stored as supposed but unfortunately they're not. I was getting Permanent error: Unknown destination address on my console.

Permanent Error: rpc_clients:2113] JsonRpcError { client_info: "coin: RICK", request: 
JsonRpcRequest { jsonrpc: "2.0", id: "34", method: "blockchain.block.headers", params: 
[Number(2), Number(16)] }, error: Internal("Unknown destination address 
electrum3.cipig.net:10017") }, on retrieving headers from server electrum3.cipig.net:10017

@borngraced I guess this was during coin activation not after since it tries to get blocks 2-16, you should have checked task::enable_utxo::status to know that the coin wasn't activated and returned this error which is the desired approach, since if there was a problem with one of the electrum servers on activation, this error indicates that the user shouldn't use this electrum server. Please remove electrum3.cipig.net:10017 from the electrum list and try again, please also provide me with the logs that led to that problem without removing electrum3.cipig.net:10017 to make sure that the problem was from the server, in the logs there should be a log that indicates why the url was removed from the connections list.

Weird, but it's working fine now. tested with BTC also!

@shamardy
Copy link
Collaborator

Weird, but it's working fine now. tested with BTC also!

There is an ignored test that tests the BTC block headers sync that I always test before pushing :), that's how I knew this was working
https://github.com/KomodoPlatform/atomicDEX-API/blob/ef82cb7b481fb9ddf60d0c81027f3fc7db0a5322/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs#L7257-L7261

Copy link
Member Author

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏼 well-done

@shamardy shamardy merged commit 1c261cd into dev May 11, 2023
@shamardy shamardy deleted the chain-reorg branch May 11, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants