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

Update coincontrol.h #182

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Update coincontrol.h #182

merged 2 commits into from
Mar 4, 2024

Conversation

mctrivia
Copy link

@mctrivia mctrivia commented Mar 1, 2024

Fix netflix bug

Fix netflix bug
@mctrivia mctrivia mentioned this pull request Mar 1, 2024
@ghost
Copy link

ghost commented Mar 1, 2024

After small change in the code. ( std:: ) i can build it. The fix is working! Ty @mctrivia
const int DEFAULT_MAX_DEPTH = std::numeric_limits<int>::max();

Screenshot with DEFAULT_MAX_DEPTH = 100;
Screenshot from 2024-03-01 20-09-56

specified that numeric_limits is part of standard
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Tested ACK

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

cACK

Thank you @mctrivia

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

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

cACK. Thanks @mctrivia!

@JaredTate
Copy link

JaredTate commented Mar 4, 2024

Thank you @mctrivia for finding this and your PR. Wouldn't it be best to add 3 more 9's to the variable to match the same proportions as found in BTC, respecting the 1:1000 DGB to BTC ratio?... well actually our block times are 40x faster... so maybe no need for 1000x more. Or was there another reason you set it to "infinity"?

const int DEFAULT_MAX_DEPTH = 9999999999;

const int DEFAULT_MAX_DEPTH = std::numeric_limits<int>::max();
This variable is set to the maximum possible value for an integer type, as defined by std::numeric_limits::max(). For a typical 32-bit integer, this value would be 2,147,483,647. It's a constant integer variable with a size of 4 bytes. Smaller in size than 9,999,999,999 would allow.

However, in this context, you've mentioned setting it to infinity, but std::numeric_limits::max() doesn't represent infinity; it represents the maximum finite value that can be stored in an int.

To represent infinity in C++, you would typically use a floating-point type and set it to std::numeric_limits<float>::infinity() or std::numeric_limits<double>::infinity()

So setting it to const int DEFAULT_MAX_DEPTH = 9999999999; would allow for coin control to go back 10 billion blocks vs 2.1 billion. Which we are a long way from both. But might as well only bump this once. Cheers

@mctrivia
Copy link
Author

mctrivia commented Mar 4, 2024

@JaredTate technically you are correct it is not infinity. But it is as close to infinity as possible with the restriction that the input is a 32bit integer. Keeping as a limit makes obvious that if the variable gets switched to 64bit that this should change also.

Admittedly using 15 sec timing your suggestion of adding 3x 9s will work for another 4000 years, but if the timing changes or the coin gets really old it will reissue this bug in the future and it may get missed.

*edit actually there isn't room in a 32 bit integer to add 3x 9s so that isn't even possible.

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

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

Fair point, we will all be dead and AI will have taken over humanity with quantum computers and will have gone interstellar by the time this matters.

ACK. Compiles and runs as expected, though I do not have an old wallet to test the main issue this fixes but I am sure we will get the chance with this bull run as people come out of the woodwork.

@ycagel ycagel merged commit 879e6d8 into DigiByte-Core:develop Mar 4, 2024
@Sbosvk
Copy link

Sbosvk commented Mar 5, 2024

For what it's worth, this has been tested by going in the opposite direction and setting a ridiculously low number. You can see the conversation on this Discord thread: https://discord.com/channels/878200503815782400/1212897730129825883

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