fix(electrum): fixed chain sync issue#1163
Closed
evanlinjin wants to merge 1 commit intobitcoindevkit:masterfrom
Closed
fix(electrum): fixed chain sync issue#1163evanlinjin wants to merge 1 commit intobitcoindevkit:masterfrom
evanlinjin wants to merge 1 commit intobitcoindevkit:masterfrom
Conversation
5a902d5 to
b2ccba3
Compare
* Fixed a logic error in `construct_update_tip` that caused the local chain tip to always be a block behind the actual tip. * Docs are added to clarify the `construct_update_tip` logic. * `ASSUME_FINAL_DEPTH` is renamed to `MAX_REORG_DEPTH`. Testing: This is tested manually (we need to add a proper test framework in the near future). `example_electrum scan` can detect incoming transactions when unconfirmed, and detect that is becomes confirm in later calls. `exampl_electrum sync` can do that same. Co-authored-by: Wei Chen <wzc110@gmail.com>
b2ccba3 to
55ad9e8
Compare
This was referenced Oct 10, 2023
danielabrozzoni
approved these changes
Oct 10, 2023
Member
danielabrozzoni
left a comment
There was a problem hiding this comment.
ACK 55ad9e8 - I'm still encountering the bug every now and then, but I still want to merge this PR as it fixes an obvious issue.
Collaborator
|
See comment here: #1145 (comment) I still think that the renaming here is embedding a mistaken concept. There is no Also can we please have a test that reproduces the issue and fixes it in this PR. This does not block |
Member
Author
33 tasks
38 tasks
54 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This replaces #1145 and is based on this comment: #1145 (comment). I pushed forward with this as I think it is a really good idea to include this in our
alpha.2milestone.This may or may not fix #1125 (however, neither @LagginTimes or myself can replicate the issue @danielabrozzoni has been running into - check #1145 for convo). We can confirm whether it does once we have proper tests. However, these changes here obviously fix a logical flaw.
construct_update_tipthat caused the local chain tip to always be a block behind the actual tip.construct_update_tiplogic.ASSUME_FINAL_DEPTHis renamed toMAX_REORG_DEPTH.Notes to the reviewers
This is tested manually (we need to add a proper test framework in the near future). I can perform the following successfully:
example_electrum scancan detect incoming transactions when unconfirmed, and detect that is becomes confirm in later calls.exampl_electrum synccan do that same.Changelog notice
construct_update_tipthat caused the local chain tip to always be a block behind the actual tip.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes:
* [ ] This pull request breaks the existing API* [ ] I've added tests to reproduce the issue which are now passing