Page MenuHomePhabricator

Change default for key fee transactions policies
AbandonedPublic

Authored by jasonbcox on Sep 22 2017, 15:42.

Details

Reviewers
freetrader
deadalnix
tomtomtom7
dagurval
schancel
hanchon
sickpig
Group Reviewers
Restricted Project
Summary

These are the parameters we changed the default to:

  1. blockmintxfee from 1000 satoshi/kB to 0
  2. minrelaytxfee from 1000 satoshi/kB to 0
  3. limiterfreerelay from 0 satoshi KB/min to 20

The following is a quick description of each parameter:

  • blockmintxfee determine the lowest fee rate in BCC/KB for transactions to be included in a lock creation.
  • Fee smaller than minrelaytxfee (BCC/kB) than this are considered zero fee for relaying, mining and transaction creation.
  • Continuously rate-limit free transactions to`limiterfreerelay`*1000 bytes per minute.
  • change the default for -relaypriority to false. That means that free transaction could be relayed even if they are not high-prio

depends on D856

Test Plan

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

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D554
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1459
Build 1459: arc lint + arc unit

Event Timeline

sickpig retitled this revision from Change default for key fee transactions policies to [WIP] Change default for key fee transactions policies.
sickpig edited the summary of this revision. (Show Details)

Add WIP to title a list of TODO taks

src/miner.cpp
106 ↗(On Diff #1403)

Why is ParseMoney no longer required?

src/validation.h
145 ↗(On Diff #1403)

Would you mind adding a comment about this parameter and it's units?

deadalnix requested changes to this revision.Dec 5 2017, 14:28

Back on your queue.

This revision now requires changes to proceed.Dec 5 2017, 14:28
src/miner.cpp
106 ↗(On Diff #1403)

you are right I should not have removed. going to readd it.

src/validation.h
145 ↗(On Diff #1403)

will do

sickpig marked 2 inline comments as done.Dec 7 2017, 09:54
sickpig marked 2 inline comments as done.Dec 7 2017, 10:01

Add comment to explain what DEFAULT_LIMITFREERELAY is

These set of changes fixed a runtime error occuring during the execution of miner_tests.
It was due to the use of ForceSetArg in TestPackageSelection to change the default value of blockMinFeeRate.

These are the list of changes:

  • Add wrap function to set blockMinFeeRate private variable
  • Init blockMinFeeRate to CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE) at declaration time rather than in BlockAssembler
  • Using the same BlockAssembler object across all TestPackageSelection to avoid to reinit blockMinFeeRate everytime.
  • Add unit test to verify that 0 fee non high priority transactions are selected for inclusion in a block.
  • let minrelaytxfee to be set to 0.

Looks good. I need to dig into this section of the code more before I feel comfortable signing off.

src/miner.h
138 ↗(On Diff #2139)

Why did you choose to initialize this here rather than in the constructor?

src/test/miner_tests.cpp
118 ↗(On Diff #2139)

Does this need to be reset at the end of the test? It looks like it's defined globally.

Change default for key fee transactions policies

  • modify sendrawtransaction (in src/rpc/rawtransaction.cpp) so that locally generated transaction is rate limited accordinf to limiterfreerelay
  • change the default for -relaypriority to false. That means that free

transaction could be relayed even if they are not high-prio

  • added unit test to verify that 0 fee non high priority

transactions are selected for inclusion in the block.

  • added functional test to verify that local generated transactions are rate

limited as per limiterfreerelay

  • add functional test to verify that limiterfreerelay is actually enforced

for transactions generated from other nodes.

sickpig retitled this revision from [WIP] Change default for key fee transactions policies to Change default for key fee transactions policies.Dec 27 2017, 22:30
sickpig edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Dec 28 2017, 12:17

If you simply start allowing this, then we can be sure we are going to get blocks stuffed with garbage just to make a point.

Free transaction needs to be gated by other, non fakable parameters, such as coinage.

This revision now requires changes to proceed.Dec 28 2017, 12:17

I partly agree, but we have to take into consideration that theoretically it is not possible to relay more than 20KB of free transactions per minute. Hence 200KB worth of free transactions per block.

Either way if we think that coinage (and tx size) will be enough to avoid a tragedy of the common than we just need to set -relaypriority default back to true and be sure that it will be enforced for locally generated transactions and for txns that arrive from external peers.

That way we are going to relay only transactions that will get a priority higher than:

inline double AllowFreeThreshold() {
    return COIN.GetSatoshis() * 144 / 250;
}

What we probably need think is a way to tweak those arbitrary number (is 144 confirmations and 250 bytes enough)?

Other than that the diff made two independent useful canges:

  • add a test to check if limitfreerelay policy is enforced, it was not present before
  • previously txns locally generated by sendrawtransaction weren't subjected to limitfreerelay limit.

Looking more closely, this diff is really doing many things at once. This is a bad for many reasons, but maybe the most obvious one is that as long as one of these element has some question mark on it, all of them are blocked and no progress is made. In addition, it makes the diff harder to review, which increase the risk of mistakes and makes the first problem significantly worse. I left various comment on what seems like changes that could go in on their own. Doing these changes in their own diff would ensure that they can be merged quickly, but would also make this is one significantly simpler.

src/init.cpp
1474

So this parameter sets what is considered to be zero fee. It sounds like this can be a changed on its own and is not dependent on the change of other parameters.

src/miner.h
165

It's not clear why this setter is needed. The config is passed in at construction, and it sounds like blockmintxfee fit right there. This sounds like a change that should go in on its own.

src/rpc/rawtransaction.cpp
1116

It's very unclear what this is and what this do. However, it seems like it is a change in the behavior of this RPC call, and would deserve a diff of its own.

test/functional/test_runner.py
51

This is the only change, right ?

338

Please submit the formatting changes to this file separately and then rebase this diff on top fo it.

src/init.cpp
1474

Agreed going to submit on another diff

src/miner.h
165

did it that way because it was impossible to change blockmintxfee via ForceSetArg once the object was instantiated. It lead to a SEGABRT.

sickpig@host:~/src/bitcoin-abc(arcpatch-D554)$> src/test/test_bitcoin --run_test=miner_tests
Running 3 test cases...
unknown location(0): fatal error in "CreateNewBlock_validity": memory access violation at address: 0x0000005c: no mapping at fault address
test/miner_tests.cpp(134): last checkpoint

*** 1 failure detected in test suite "Bitcoin Test Suite"
test_bitcoin: /usr/include/boost/thread/pthread/condition_variable_fwd.hpp:81: boost::condition_variable::~condition_variable(): Assertion `!ret' failed.
Aborted (core dumped)

for more details see:
https://reviews.bitcoinabc.org/D554#14275

138 ↗(On Diff #2139)

No particular reason. I thought that it would make the init code block in the constructor easie, but on second thought it's clearer to have it the initialization all in one place.

src/rpc/rawtransaction.cpp
1116

sendrawtransaction is used to submit locally generated transactions to the mempool during unit test.

Having fLimitFree set to false means that the limiterfreerelay constraint was not applied to txns submitted via sendrawtransaction.

It seems a bug to me to have a different policy for the acceptance of locally generate txns and remotely generated ones (received via another peer)

src/test/miner_tests.cpp
118 ↗(On Diff #2139)

All the tests in this unit assume blockMinFeeRate is set to Amount(1000) that's the reason why I set it here once I set it to `Amount(0) in the test I added just before to this one (see L90 of this file)

test/functional/test_runner.py
51

yes it is.

going to push another diff to autopep test_runner and I'll rebase on top of it.

rebased on top of master and D856

It seems a bug to me to have a different policy for the acceptance of locally generate txns and remotely generated ones (received via another peer)

Is this still a problem we need to address?

schancel requested changes to this revision.Jul 12 2018, 18:28
schancel added inline comments.
src/rpc/rawtransaction.cpp
1116 ↗(On Diff #2279)

Well, locally generated transactions should be able to be mined by local nodes if that's what the miner wants. Other miners may not relay the transaction though. The previous value does seem sane. I'm not sure why this is even configured.

test/functional/.gitignore
3 ↗(On Diff #2279)

This should already be fixed from my other patch

This revision now requires changes to proceed.Jul 12 2018, 18:28
jasonbcox abandoned this revision.
jasonbcox added a reviewer: sickpig.

Need to clear up the review queue, and this change is very old. Please reopen if development continues.