-
Notifications
You must be signed in to change notification settings - Fork 94
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
[r2r] spv storage optimization #1 #1585
Conversation
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
# Conflicts: # mm2src/coins/utxo/utxo_builder/utxo_arc_builder.rs # mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just a few comments to start the review, I will have more comments in next iterations.
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! Next review iteration.
@borngraced Can you please add a test similar to test_btc_block_header_sync
but with optimization? you can use a recent retarget header as the starting block header and max_stored_block_headers=RETARGETING_INTERVAL
. This test will not be ignored since the block headers sync time will be short.
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next review iteration!
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next Iteration! I believe the next review will be the last one :)
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will be the last review iteration :)
Only some minor changes left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for your patience with all the fixes @borngraced on this very tricky PR :)
@sergeyboyko0791 Can you please take a look at this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the great enhancement!
I have multiple comments
"spv_conf": { | ||
"starting_block_header": { | ||
"height": 764064, | ||
"hash": "00000000000000000006da48b920343944908861fa05b28824922d9e60aaa94d", | ||
"bits": 386375189, | ||
"time": 1668986059, | ||
}, | ||
"max_stored_block_headers": 3000, | ||
"validation_params": { | ||
"difficulty_check": true, | ||
"constant_difficulty": false, | ||
"difficulty_algorithm": "Bitcoin Mainnet" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this break the backward compatibility?
cc: @shamardy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coins repo should be updated before merging this. Only BTC
have spv configuration now in the coins file, and I think this change will make it easier to get the starting block header info from any block explorer unlike how it used to be with block header hex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info! Hope there will not be any problem with it
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change please
Signed-off-by: borngraced <samiodev@icloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove! @borngraced can you please post in the related issue the changes required to the coins configs to be merged with this?
@ca333 I believe this can be merged to dev. Will require an update to |
issue: #1483
This PR introduces new
SPVConf
structures with some fields that's needed to optimize stored block headers whenspv proof
is enabledMain Optimization in this PR is allowing node to set max headers that needs to be stored(
max_stored_block_headers
) and where to start fetching block headers from(block_header
).