Describe the issue
CreateNewBlock() has logic to sort the transactions in the mempool into a priority queue, to have the highest fee transactions first. The code appears to then be intended to fill to a configurable minimum block size with free transactions.
The default for minimum block size is 0; so logically no free transactions should be picked up; however free transactions are picked up; and it will fill to the maximum size of the block.
Expected behavior
With the current default; no free transactions should be picked up. This should be configurable with the blockminsize option.
Actual behavior
There is no limit (aside of maximum block size) to the number of free transactions that are added to the block. Upon initial inspection, this appears to be due to the priority size option. (blockprioritysize) which is defaulted to 50k.
if (!tx.HasZerocoinSpendInputs() && fSortedByFee && (dPriorityDelta <= 0) && (nFeeDelta <= 0) && (feeRate < ::minRelayTxFee) && (nBlockSize + nTxSize >= nBlockMinSize))
The above code appears to indicate that if it's a free transaction, and the block is greater than the BlockMinSize, and it's sorted by fee; then don't include it. However I believe fSortedByFee is not set correctly:
bool fSortedByFee = (nBlockPrioritySize <= 0);
If the nBlockPriority size is zero or less (zero), then set fSortedByFee to be true; otherwise set it to false. Since the default is 50k; then this will be false unless someone specifically runs with -blockprioritysize=0. This seems inverse to what it should be.
I would think that with a block priority size set; then the transactions in memory pool should be sorted by the fee; and if the block has not yet reached min size; fill with free transactions until that min size is reached.
e.g, I believe it should be 'fSortedByFee = (nBlockPrioritySize > 0)'.
Additionally, the blockminsize should be raised from zero to a different value; 1000, 5000? An analysis of average traffic could be used to determine a good default number.
With the addition of #970 ; specifically the portion to remove stale wallet transactions will assist users to be able to get back their free transactions that aren't getting picked up anymore. Additionally, blockminsize can be adjusted by the community if abuse of free transactions is found to be present on the network.
There is still some other questionable code to test; to confirm the other checks in the rejection of a free transactions are not also incorrectly false. Due to the need for transactions to be present when creating the block, the ability to test this and investigate on testnet is difficult, and I will need to setup a standalone test chain to have control over the staking as well as a mix of free and fee transactions in mempool. However, if my above assessment is correct; then free transactions can be currently "turned off" individually by a staker by defining blockprioritysize=0.
Because of the challenge of testing and validation of my assessment and fix; I am submitting this issue to gather some feedback on my review before going too far down a rabbit hole.
Describe the issue
CreateNewBlock()has logic to sort the transactions in the mempool into a priority queue, to have the highest fee transactions first. The code appears to then be intended to fill to a configurable minimum block size with free transactions.The default for minimum block size is 0; so logically no free transactions should be picked up; however free transactions are picked up; and it will fill to the maximum size of the block.
Expected behavior
With the current default; no free transactions should be picked up. This should be configurable with the
blockminsizeoption.Actual behavior
There is no limit (aside of maximum block size) to the number of free transactions that are added to the block. Upon initial inspection, this appears to be due to the priority size option. (
blockprioritysize) which is defaulted to 50k.if (!tx.HasZerocoinSpendInputs() && fSortedByFee && (dPriorityDelta <= 0) && (nFeeDelta <= 0) && (feeRate < ::minRelayTxFee) && (nBlockSize + nTxSize >= nBlockMinSize))The above code appears to indicate that if it's a free transaction, and the block is greater than the BlockMinSize, and it's sorted by fee; then don't include it. However I believe
fSortedByFeeis not set correctly:bool fSortedByFee = (nBlockPrioritySize <= 0);If the nBlockPriority size is zero or less (zero), then set fSortedByFee to be true; otherwise set it to false. Since the default is 50k; then this will be false unless someone specifically runs with -blockprioritysize=0. This seems inverse to what it should be.
I would think that with a block priority size set; then the transactions in memory pool should be sorted by the fee; and if the block has not yet reached min size; fill with free transactions until that min size is reached.
e.g, I believe it should be 'fSortedByFee = (nBlockPrioritySize > 0)'.
Additionally, the blockminsize should be raised from zero to a different value; 1000, 5000? An analysis of average traffic could be used to determine a good default number.
With the addition of #970 ; specifically the portion to remove stale wallet transactions will assist users to be able to get back their free transactions that aren't getting picked up anymore. Additionally, blockminsize can be adjusted by the community if abuse of free transactions is found to be present on the network.
There is still some other questionable code to test; to confirm the other checks in the rejection of a free transactions are not also incorrectly false. Due to the need for transactions to be present when creating the block, the ability to test this and investigate on testnet is difficult, and I will need to setup a standalone test chain to have control over the staking as well as a mix of free and fee transactions in mempool. However, if my above assessment is correct; then free transactions can be currently "turned off" individually by a staker by defining
blockprioritysize=0.Because of the challenge of testing and validation of my assessment and fix; I am submitting this issue to gather some feedback on my review before going too far down a rabbit hole.