Set default minimum block space reserved for high priority txns to 5% of the max generated block size, i.e. 50K per MB.
Details
Details
make check && qa/qa/pull-tester/rpc-tests.py -extend
Diff Detail
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
Comment Actions
@deadalnix as I wrote in the note I still have to add the test to verify that hiprio txns will be actually included.
Comment Actions
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. |
Comment Actions
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 |
Comment Actions
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 |
| 70 ↗ | (On Diff #1290) | "chunk" --> "test step" please |
| 86 ↗ | (On Diff #1290) | "second test step" |
Comment Actions
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
Comment Actions
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 | |
| 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? | |
| src/policy/policy.h | ||
| 22 | Ok what about: | |
| 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. | |
| 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. |
| src/init.cpp | ||
|---|---|---|
| 1279 ↗ | (On Diff #1307) | changed blkprio to int64_t |
Comment Actions
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. | |
| src/miner.cpp | ||
|---|---|---|
| 566 | good idea. going to do it. | |
Comment Actions
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].
| src/miner.cpp | ||
|---|---|---|
| 566 | it should be done see the last rev (1319) | |
Comment Actions
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. |
Comment Actions
Remove commented log statement in high_priority_transaction.py
Avoid passing config to addPriotityTxs as a parameter use the member in the object