Details

Reviewers

freetrader
deadalnix
tomtomtom7
dagurval
schancel
hanchon
sickpig

Group Reviewers

Restricted Project

Summary

These are the parameters we changed the default to:

blockmintxfee from 1000 satoshi/kB to 0
minrelaytxfee from 1000 satoshi/kB to 0
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 created this revision.Sep 22 2017, 15:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 22 2017, 15:42

Add WIP to title a list of TODO taks

Harbormaster completed remote builds in B882: Diff 1403.Sep 22 2017, 15:50

schancel added inline comments.Oct 3 2017, 03:55

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?

Back on your queue.

This revision now requires changes to proceed.Dec 5 2017, 14:28

sickpig added inline comments.Dec 7 2017, 09:47

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

Rebased

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.

Harbormaster completed remote builds in B1317: Diff 2036.Dec 11 2017, 16:25

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.

Harbormaster completed remote builds in B1383: Diff 2139.Dec 15 2017, 14:26

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.

Harbormaster completed remote builds in B1459: Diff 2257.Dec 27 2017, 22:27

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)

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.

sickpig added inline comments.Dec 31 2017, 15:48

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.

sickpig edited the summary of this revision. (Show Details)Jan 2 2018, 13:16

sickpig added a parent revision: D856: Permit to set `minrelaytxfee` to 0.

rebased on top of master and D856

Harbormaster completed remote builds in B1477: Diff 2279.Jan 2 2018, 13:16

sickpig mentioned this in D856: Permit to set `minrelaytxfee` to 0.Jan 2 2018, 15:19

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

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

Change default for key fee transactions policies
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 2257

src/init.cpp

src/miner.h

src/miner.cpp

src/policy/policy.h

src/rpc/rawtransaction.cpp

src/test/miner_tests.cpp

src/validation.h

test/functional/.gitignore

test/functional/relay_zero_fee_transaction.py

test/functional/test_runner.py

Change default for key fee transactions policiesAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2257

src/init.cpp

src/miner.h

src/miner.cpp

src/policy/policy.h

src/rpc/rawtransaction.cpp

src/test/miner_tests.cpp

src/validation.h

test/functional/.gitignore

test/functional/relay_zero_fee_transaction.py

test/functional/test_runner.py

Change default for key fee transactions policies
AbandonedPublic
Actions

Revision Contents
Changeset List