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

Ensure Height is at least SegWit activation #12192

Merged

Conversation

turbolay
Copy link
Collaborator

@turbolay turbolay commented Jan 5, 2024

Fixes a bug revealed by #12185.

var startingSegwitHeight = new Height(SmartHeader.GetStartingHeader(Network, IndexType.SegwitTaproot).Height);
if (startingSegwitHeight > bestKeyManagerHeight)
{
KeyManager.SetBestHeights(startingSegwitHeight, startingSegwitHeight);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method is always called with: "X, X" parameters (the same value for both parameters). It would be probably better to have just one parameter and set both heights in that method to the same value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. I can do it in another PR after this one is merged

@@ -471,6 +473,14 @@ private async Task LoadWalletStateAsync(CancellationToken cancel)

Height bestKeyManagerHeight = KeyManager.GetBestTurboSyncHeight();

// Make sure best height is at least height of segwit activation.
var startingSegwitHeight = new Height(SmartHeader.GetStartingHeader(Network, IndexType.SegwitTaproot).Height);
if (startingSegwitHeight > bestKeyManagerHeight)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for this PR but maybe we should implement comparison with uint types as well.

@kiminuo
Copy link
Collaborator

kiminuo commented Jan 5, 2024

rant: Not for this PR as well, but we should IMO try to convert BlockchainState heights to uints. It would be better because there is no Mempool height for that case. It would be a nice PR and we would remove a bunch of (u)int <-> Height conversions that just cost performance and dev time (thinking about what values are allowed where).

LGTM

@turbolay
Copy link
Collaborator Author

turbolay commented Jan 5, 2024

Not for this PR as well, but we should IMO try to convert BlockchainState heights to uints

Agreed, it's always a struggle of conversions between these when writing synchronization code. I opened an issue about it: #12194

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

LGTM - just one puzzler to solve.

WalletWasabi/Wallets/Wallet.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

tACK

@molnard molnard merged commit 0b58884 into WalletWasabi:master Jan 6, 2024
7 checks passed
@turbolay turbolay deleted the ensureHeightIsSegWitActivation branch February 26, 2024 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants