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

Fix for GetBlockValue() after block 13,440,000 #3842

Merged
merged 2 commits into from
Apr 3, 2014

Conversation

ditto-b
Copy link
Contributor

@ditto-b ditto-b commented Mar 11, 2014

Forces the block reward to zero when right shift in GetBlockValue() is
undefined, after 64 reward halvings (block height 13,440,000).

Forces the block reward to zero when right shift in GetBlockValue() is
undefined, after 64 reward halvings (block height 13,440,000).
@petertodd
Copy link
Contributor

NACK

We should make use of the ~250 years we have to study this issue more carefully.

@bf0d7998-81ec-48d1-b236-076ed3c77581

I'm sure the altcoin creators can figure this out of themselves right?

@ditto-b
Copy link
Contributor Author

ditto-b commented Mar 12, 2014

UUID guy is giving altcoin devs way too much credit.

@laanwj
Copy link
Member

laanwj commented Mar 14, 2014

Code change looks fine to me.
Any value of halvings>32 will already bring the subsidy to 0, so regarding it as zero for halvings>=64 is safe.

@laanwj
Copy link
Member

laanwj commented Apr 1, 2014

Are we going to merge this, or not? It appears safe to me but as it involves critical part of the consensus code I don't want to make this decision on my own.
@sipa @gmaxwell @gavinandresen

@petertodd
Copy link
Contributor

@ditto-b Could you please do up a test script that actually exercises this code? regtest mode has a shortened halving interval and a trivial PoW so creating the blocks and submitting them locally shouldn't be too hard. You can use https://github.com/petertodd/python-bitcoinlib/ if you know python - it has both the CBlock/CTransaction stuff to make the blocks, and easy access to the RPC interface. I'd be happy to help review it for you.

@laanwj
Copy link
Member

laanwj commented Apr 1, 2014

We do have a test for the function (subsidy_limit_test) but it only runs until block 7 000 000. Would be trivial to increase, though.

@petertodd
Copy link
Contributor

@laanwj Gah! I was trying to get @ditto-b to write a test harness for me to use to test the upcoming Litecoin soft-fork.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 1, 2014

ACK. This is an implementation of BIP42.

Though we should make the trivial change of improving the test too.

@gergomiho
Copy link

I think this is a great fix. When I first read about Bitcoin I loved the idea that it has finite coins, and this is how it should be in my opinion.

@genjix
Copy link

genjix commented Apr 2, 2014

:S in 250 years we'll be using qubits, flying around the galaxy and mating with space honeys

i'm glad you have eternal faith in c++

@parentmap
Copy link

Code is moving away from it's comments. Keep the comment about halving with the actual halving math (l 1085 -> 1079)

@aceat64
Copy link

aceat64 commented Apr 2, 2014

This is clearly a bug and one serious enough that I believe if we do not fix it in a timely manner it could destroy confidence in the Bitcoin network.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 2, 2014

Timely manner? ... Well I'm absolutely confident that it will be fixed sometime in the hundreds of years required before it matters.

@aceat64
Copy link

aceat64 commented Apr 2, 2014

The longer we delay, the more likely it is that a miner alive today could live until this bug occurs. This gives them a greater incentive to not implement the fix. This is particularly true for any toddlers or infants in charge of sizeable farms.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 2, 2014

It doesn't matter what miners implement, it matters what the rest of bitcoin nodes run. If you mine with rules inconsistent with the network you are simply not mining.

@laanwj
Copy link
Member

laanwj commented Apr 2, 2014

@aceat64 So, have you tested this pull? Have you updated the unit test which is the only thing that's holding this back from being merged? Please do something useful instead of distracting people by screaming hurry hurry hurry.

@aceat64
Copy link

aceat64 commented Apr 2, 2014

I feel as though the tone of my message has been misunderstood. I attempted to clarify with my mention of toddlers and infants running large mining farms.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 2, 2014

Thanks. :)

Because no one wants 4 gold mines being discovered every mibillenium.
@rebroad
Copy link
Contributor

rebroad commented Apr 3, 2014

I'm not particularly concerned about what happens to bitcoin in 250 years as I don't expect to be around by then. Is this really likely to be the PR disaster that some people are suggesting?!

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5cfd3a70a67ba707a8f074a1730724a6e86353b8 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen added a commit that referenced this pull request Apr 3, 2014
Fix for GetBlockValue() after block 13,440,000
@gavinandresen gavinandresen merged commit 8556b02 into bitcoin:master Apr 3, 2014
@sipa sipa mentioned this pull request Jun 15, 2014
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 13, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet