Skip to content

Fix nPayFee calculation#3932

Closed
Gabrola wants to merge 1 commit intobitcoin:masterfrom
Gabrola:master
Closed

Fix nPayFee calculation#3932
Gabrola wants to merge 1 commit intobitcoin:masterfrom
Gabrola:master

Conversation

@Gabrola
Copy link
Copy Markdown

@Gabrola Gabrola commented Mar 21, 2014

nPayFee is more than intended if nBytes is exactly in multiples of 1000
bytes

nPayFee is more than intended if nBytes is exactly in multiples of 1000
bytes
@ghost
Copy link
Copy Markdown

ghost commented Mar 21, 2014

Would changing this to /1023 instead of /1000 result in faster code as the compiler could reduce this to a series of bit shifting operations?

@luke-jr
Copy link
Copy Markdown
Member

luke-jr commented Mar 21, 2014

@Gabrola What makes you think it's more than intended?

@NanoAkron It'd need to be /1024

@Gabrola
Copy link
Copy Markdown
Author

Gabrola commented Mar 21, 2014

@NanoAkron If you meant 1024, that still wouldn't work on really large transactions since the rule is 1,000 bytes not 1kB
@luke-jr nPayFee would be nTransactionFee * 2 if nBytes was 1000 when it should be just nTransactionFee * 1

@luke-jr
Copy link
Copy Markdown
Member

luke-jr commented Mar 21, 2014

@Gabrola My question is why you think it should be * 1.

@Gabrola
Copy link
Copy Markdown
Author

Gabrola commented Mar 21, 2014

The transaction relay rule states:
Base fee is: 10,000 satoshis per 1,000 bytes, transaction size rounded UP to the nearest 1,000 bytes (e.g. a 1,001 byte transaction requires a 20,000 satoshi fee). Can be changed with the -minrelaytxfee setting.

I don't think this implies that 1,000 bytes should round up to a 20,000 satoshi fee as well.

@Gabrola Gabrola changed the title Fix nPayFee Fix nPayFee calculation Mar 22, 2014
@BitcoinPullTester
Copy link
Copy Markdown

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6166d342ad6db2ffff101f70268d17e91de1d117 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.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 31, 2014

Is this consistent with the transaction selection in the mining code?

@Gabrola
Copy link
Copy Markdown
Author

Gabrola commented Mar 31, 2014

I just checked and it's not consistent with GetMinFee()
This means we'll need to change GetMinFee() first but that means we'll have to wait for the majority of miners to be using 0.9.99 before we change the client code.
Should we change GetMinFee() for now or just ignore the problem considering the unlikeliness of transaction sizes being exactly divisible by 1000?

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Apr 1, 2014

That's not worth it IMO. The fees system is being reworked any way with floating fees.

As long as GetMinFee and the wallet are consistent, it's fine at it is.

Thanks for checking.

@laanwj laanwj closed this Apr 1, 2014
@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.

4 participants