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

backport: bitcoin#18152 #5804

Merged
merged 1 commit into from
Jan 11, 2024
Merged

backport: bitcoin#18152 #5804

merged 1 commit into from
Jan 11, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jan 8, 2024

Issue being fixed or feature implemented

It splits from #5802 due to non-trivial decisions regards updateNumBlocks usage

What was done?

Backport bitcoin#18152

How Has This Been Tested?

Run unit/functional tests; tried to sync blockchain; reindex and watch a spinner and progress bar.

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6
Copy link

UdjinM6 commented Jan 8, 2024

Previous discussion is here #5802 (comment).

I think the cleanest way to fix this is to introduce another SynchronizationState e.g. RESET and use it df3d6cd.

Things to test from Console once qt node is fully synced:

  1. mnsync reset
  2. setnetworkactive false/setnetworkactive true

My results:
073b8e8: No status bar updates
df3d6cd: Works fine just like it did before this PR

@knst
Copy link
Collaborator Author

knst commented Jan 8, 2024

I just realised, that a function setNumBlocks is relevant only for Qt app, and sync_state is used only inside it. let's pass INIT_DOWNLOAD indeed, it is workaround but it would work same.

@UdjinM6
Copy link

UdjinM6 commented Jan 8, 2024

yeah, technically both these cases are similar to switching back to IBD so using INIT_DOWNLOAD kind of makes sense here too.

@knst knst added this to the 20.1 milestone Jan 9, 2024
@knst knst requested a review from PastaPastaPasta January 9, 2024 13:37
@knst knst marked this pull request as draft January 9, 2024 13:50
Copy link

This pull request has conflicts, please rebase.

@knst knst marked this pull request as ready for review January 10, 2024 22:02
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK (rebase looks clean, tested previous commit)

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

… GUI

a0d0f1c refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df7701 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)

Pull request description:

  This PR is a followup of bitcoin#18121 and:
  - addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](bitcoin#18121 (comment)), **ryanofsky**'s [comment](bitcoin#18121 (comment)))
  - removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See:  **ryanofsky**'s [comment](bitcoin#18121 (review))

ACKs for top commit:
  jonasschnelli:
    Core Review ACK a0d0f1c (modulo [question](bitcoin#18152 (review))).
  ryanofsky:
    Code review ACK a0d0f1c. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)

Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
@PastaPastaPasta PastaPastaPasta merged commit 6723381 into dashpay:develop Jan 11, 2024
5 checks passed
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.

4 participants