Skip to content

Add -blockmintxfee setting#1097

Merged
zeptin merged 6 commits intostratisproject:release/1.5.0.0from
zeptin:blockmintxfee-20221130
Dec 4, 2022
Merged

Add -blockmintxfee setting#1097
zeptin merged 6 commits intostratisproject:release/1.5.0.0from
zeptin:blockmintxfee-20221130

Conversation

@zeptin
Copy link
Collaborator

@zeptin zeptin commented Dec 1, 2022

Currently transactions that fall below the relay fee rate minimum of 0.0001 STRAX per KB can still be mined into blocks. However, the block construction has a further (currently hardcoded) limit of 0.00001 STRAX per KB. Transactions that are below this fee rate will never be included in blocks even if they make it into a node's mempool. This change makes this lower limit configurable, similarly to the -blockmintxfee setting in bitcoind. The default value has been kept.

It would have been nice to remove the PowMining.DefaultBlockMinTxFee value entirely and base the default minimum fee rate on the node relay policies, but the value gets used in several tests and it was cleaner to keep it for now to tie all the 'magic numbers' together. It is also unclear whether increasing the block fee minimum tenfold to match the relay minimum would have any adverse impact.

The dumpprivkey RPC command commits also seem to be missing from the 1.5.0.0 branch, so they have been brought in here.

@zeptin zeptin added the 1.6.0.0 label Dec 1, 2022
@zeptin zeptin requested a review from quantumagi December 1, 2022 10:16
@quantumagi
Copy link
Contributor

quantumagi commented Dec 2, 2022

@zeptin , Looks good. Is the dumpprivkey a possible security risk and if not why not?

@zeptin
Copy link
Collaborator Author

zeptin commented Dec 2, 2022

@quantumagi I don't think it changes the security assumptions of the node. Similarly to sending transactions (or staking) via RPC it requires the wallet to be unlocked beforehand in order to work. If an external entity has access to the node via RPC while the wallet is unlocked they could already make transactions, this PR doesn't change that. Note that the functionality of the RPC is similar to the privatekey endpoint in the wallet controller, although that endpoint explicitly requires a password to be provided.

@quantumagi
Copy link
Contributor

Would it detract from the usability of this endpoint if we require an explicit password to be passed?

@zeptin
Copy link
Collaborator Author

zeptin commented Dec 3, 2022

Not much point, the node has an RPC password anyway. Adding a further password to the RPC parameters breaks compatibility:

https://developer.bitcoin.org/reference/rpc/dumpprivkey.html

Copy link
Contributor

@quantumagi quantumagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zeptin zeptin merged commit bdc9f2d into stratisproject:release/1.5.0.0 Dec 4, 2022
@zeptin zeptin deleted the blockmintxfee-20221130 branch December 4, 2022 17:12
quantumagi pushed a commit to quantumagi/StratisFullNode-1 that referenced this pull request Jan 31, 2023
* Add dumpprivkey RPC command

* Revert superfluous change

* Add additional sanity test

* Fix test

* Add -blockmintxfee setting

* Fix settings property usage

(cherry picked from commit bdc9f2d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants