Page MenuHomePhabricator

Set default space reserved for hiprio txns to 5% of max generated block size
ClosedPublic

Authored by sickpig on Aug 25 2017, 12:29.

Details

Summary

Set default minimum block space reserved for high priority txns to 5% of the max generated block size, i.e. 50K per MB.

Test Plan

make check && qa/qa/pull-tester/rpc-tests.py -extend

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 25 2017, 12:29
sickpig updated this revision to Diff 1213.Aug 25 2017, 13:35

Properly set up -blockprioritysize default

deadalnix requested changes to this revision.Aug 26 2017, 12:50

It doesn't looks like the behavior when the flag is enabled is tested in any way.

This revision now requires changes to proceed.Aug 26 2017, 12:50
sickpig added a comment.Aug 28 2017, 20:58

@deadalnix as I wrote in the note I still have to add the test to verify that hiprio txns will be actually included.

sickpig updated this revision to Diff 1258.Aug 28 2017, 21:04
sickpig edited edge metadata.

Add blockprioritypercentage parameter and remove blockprioritysize

Set the percentage of the max generated block size used for high priority transactions.
The dafault value is 5, hence on a standard installation the space reserved for hiprio
txns will be 100K.

TODO:

  • add a test to check that the hi prio txns will be included in the reserved space
  • add a test to check blockprioritysize input validation (need to be into 0 .. 100)
freetrader added inline comments.Aug 30 2017, 20:14
src/miner.cpp
572 ↗(On Diff #1258)

values > 100 are invalid inputs, the user should be rapped on the knuckles and the program should not continue.

Maybe the user meant to type 20 (percent), accidentally a 0 , and now allocating 100% to it.

Not safe - input must be validated to be within correct range.

sickpig updated this revision to Diff 1290.Aug 31 2017, 15:14
sickpig retitled this revision from [WIP] Set default minimum space for hi-prio txns to 5K per MB to Set default space reserved for hiprio txns to 5% of max generated block size.
sickpig edited the summary of this revision. (Show Details)
sickpig edited the test plan for this revision. (Show Details)
sickpig added 1 blocking reviewer(s): Restricted Project.

Take out of WIP, added functional test to verify hiprio txns inclusion. Stop execution if -blockptioritypercentage par is out of range

sickpig added inline comments.Aug 31 2017, 15:16
src/miner.cpp
572 ↗(On Diff #1258)

fair point

freetrader edited edge metadata.Aug 31 2017, 17:31

Generally: I think it would be nice to have an RPC interface to allow miners to get/set the current blockprioritypercentage . This might be more involved though, so it's best dealt with in a separate diff at some stage I think (it's also not very urgent, a configuration parameter is a good start).

Please mention in Release Notes about the change from blockprioritysize -> blockprioritypercentage . I would recommend directly to update the release notes doc together with this Diff .

qa/rpc-tests/high_priority_transaction.py
67 ↗(On Diff #1290)

Please add a comment to refer to the C++ definition of AllowFreeThreshold() in src/txmempool.h
Since these numbers are pretty arbitrary, at least help to know direct relationship between the test and application code.

70 ↗(On Diff #1290)

"chunk" --> "test step" please

86 ↗(On Diff #1290)

"second test step"

freetrader requested changes to this revision.Aug 31 2017, 18:39
This revision now requires changes to proceed.Aug 31 2017, 18:39
sickpig updated this revision to Diff 1298.EditedAug 31 2017, 20:19
sickpig edited edge metadata.

Fix comments in high high_priority_transaction.py
Add comment in the new added test to explain the definition of the hiprio cut-off
Mention the introduction of blockprioritypercentage parameter in the release notes
Mention the deprecation of blockprioritysize parameter in the release notes
Fix python files indentation

sickpig marked 3 inline comments as done.Aug 31 2017, 20:20
sickpig updated this revision to Diff 1302.Aug 31 2017, 20:47

Fix typos in the release-notes.md

sickpig updated this revision to Diff 1303.Aug 31 2017, 21:39
sickpig edited the test plan for this revision. (Show Details)

Fix bip68-sequence.py

freetrader accepted this revision.Sep 1 2017, 00:32
sebicas awarded a token.Sep 1 2017, 14:59
deadalnix requested changes to this revision.Sep 1 2017, 20:22

I like where this is going but it's not quite there yet.

qa/rpc-tests/test_framework/util.py
328 ↗(On Diff #1303)

Put it before initialize_datadir(test_dir, i)

src/miner.cpp
566 ↗(On Diff #1303)

This won't accurately check for absurd sizes. You need to get the value as a uint64_t at least and check that is is smaller than or equal to 100.

src/policy/policy.h
22 ↗(On Diff #1303)

Please fix the whole comment.

src/rpc/mining.cpp
293 ↗(On Diff #1303)

Same, uint64_t

This revision now requires changes to proceed.Sep 1 2017, 20:22
sickpig added inline comments.Sep 1 2017, 21:11
qa/rpc-tests/test_framework/util.py
328 ↗(On Diff #1303)

ok

src/miner.cpp
566 ↗(On Diff #1303)

we check it at init time, do we really need to check it twice?
see line 1279 of init.cpp.

src/policy/policy.h
22 ↗(On Diff #1303)

Ok what about:
"Default for -blockprioritypercentage, determine the amount of block space reserved to high priority transactions"

src/rpc/mining.cpp
293 ↗(On Diff #1303)

I got no problem to change it, but at this time we are already sure that the value belongs to the [0..100] interval.

sickpig updated this revision to Diff 1307.Sep 2 2017, 08:19
sickpig edited edge metadata.
sickpig marked 2 inline comments as done.

Fix @deadalnix nits about comments

CCulianu accepted this revision as: CCulianu.Sep 2 2017, 21:11
CCulianu added inline comments.
src/init.cpp
1279 ↗(On Diff #1307)

You seemed to indicate that you wanted to make this a larger data type to be able to catch whatever garbage input the user might give the option without overflowing.

I don't think it's a showstopper to just leave it as int16_t -- although I am in favor of a larger int here. After all only values in the range [65536,65637] will actually produce valid overflows in the [0,100] range on x86 arch's...

src/rpc/mining.cpp
293 ↗(On Diff #1303)

I'm ok with leaving it as a uint8_t. Precondition of it being [0,100] is already enforced and defined.

sickpig added inline comments.Sep 3 2017, 20:43
src/init.cpp
1279 ↗(On Diff #1307)

I concur that increasing the date tyep from int16_t to int64_t would catch a lot of absurd values.

sickpig updated this revision to Diff 1308.Sep 3 2017, 21:01

Use a int64_t var to evaluate -blockprioritypercentage input value

sickpig marked an inline comment as done.Sep 3 2017, 21:02
sickpig added inline comments.
src/init.cpp
1279 ↗(On Diff #1307)

changed blkprio to int64_t

CCulianu added a comment.Sep 4 2017, 00:10

Excellent work, sickpig! I won't approve 100% just yet -- I'll wait for deadalnix to have one last look at it.

deadalnix added inline comments.Sep 4 2017, 12:18
src/miner.cpp
566 ↗(On Diff #1303)

Spooky action at a distance is to be avoided. This code rely on the behavior of some other piece of code while there are no direct link between the 2. This kind of code is guaranteed to explode at some point when someone changes it. Sounds like adding this to the config object would be a much better idea.

sickpig updated this revision to Diff 1310.Sep 4 2017, 16:26
sickpig marked an inline comment as done.

Rebase

sickpig added inline comments.Sep 5 2017, 10:08
src/miner.cpp
566 ↗(On Diff #1303)

good idea. going to do it.

sickpig updated this revision to Diff 1319.Sep 5 2017, 14:14

Add new nBlockPriorityPercentage globals and 2 new methods to get/set such value.
Set will also assure that the proposed value belongs to [0..100].

sickpig marked an inline comment as done.Sep 5 2017, 14:19
sickpig added inline comments.
src/miner.cpp
566 ↗(On Diff #1303)

it should be done see the last rev (1319)

deadalnix requested changes to this revision.Sep 5 2017, 19:32

A few details, but it's almost there.

qa/rpc-tests/high_priority_transaction.py
43 ↗(On Diff #1319)

Please don't leave commented logs in there. Either keep them or remove them. If this is a low information but still necessary, use log.debug

src/miner.cpp
564 ↗(On Diff #1319)

Use the member in the object, you don't need to pass it down as an argument.

src/miner.h
177 ↗(On Diff #1319)

You don't need to pass the config down, it is already a member of the object.

This revision now requires changes to proceed.Sep 5 2017, 19:32
sickpig added inline comments.Sep 5 2017, 19:41
qa/rpc-tests/high_priority_transaction.py
43 ↗(On Diff #1319)

left over, removing

src/miner.cpp
564 ↗(On Diff #1319)

k, you are right

src/miner.h
177 ↗(On Diff #1319)

ditto

sickpig updated this revision to Diff 1321.Sep 5 2017, 21:03
sickpig edited edge metadata.

Remove commented log statement in high_priority_transaction.py
Avoid passing config to addPriotityTxs as a parameter use the member in the object

deadalnix accepted this revision.Sep 5 2017, 21:04
This revision is now accepted and ready to land.Sep 5 2017, 21:04
This revision was automatically updated to reflect the committed changes.