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
Branch
highprio-tx-reserved-space
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 818
Build 818: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

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)
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 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

src/miner.cpp
572 ↗(On Diff #1258)

fair point

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"

This revision now requires changes to proceed.Aug 31 2017, 18:39
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

Fix typos in the release-notes.md

sickpig edited the test plan for this revision. (Show Details)

Fix bip68-sequence.py

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

Put it before initialize_datadir(test_dir, i)

src/miner.cpp
566

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

Please fix the whole comment.

src/rpc/mining.cpp
293

Same, uint64_t

This revision now requires changes to proceed.Sep 1 2017, 20:22
qa/rpc-tests/test_framework/util.py
328

ok

src/miner.cpp
566

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

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

src/rpc/mining.cpp
293

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 edited edge metadata.
sickpig marked 2 inline comments as done.

Fix @deadalnix nits about comments

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

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

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.

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

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

src/miner.cpp
566

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 marked an inline comment as done.

Rebase

src/miner.cpp
566

good idea. going to do it.

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

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
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 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

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