WIP: minimum data request reward to collateral percentage

[drcpu-github]

Following this discussion, I started with a PoC implementation for the minimum reward to collateral mechanism. In summary, there are two main changes to the code:

1. Introduction of a minimum_reward_collateral_percentage configuration variable which can freely be set by miners (as long as the value is higher than the related consensus constant). Essentially, this variable will dictate if a miner inserts a newly received data request into his local transaction pool.

2. Introduction of a required_reward_collateral_percentage consensus constant which is used for data request validation whenever a block is received or a new data request is created.

I currently am facing three issues and could use some insight into how to tackle them:

1. I doubt I can just add a new consensus constant given that it is used everywhere (e.g., in the communication between nodes). I did this anyway to make the PoC, but for the actual deployment I probably need to change this? How would adding a consensus constant need to be handled? Could we just make this a non-consensus, but fixed, parameter somewhere? People can obviously still change it and recompile the node, but as long as the majority of node operators does not, it's probably safe to not be a consensus constant.

2. Provided we can add a new consensus constant, in the Default implementation of TransactionsPool, I would like to set the required_reward_collateral_percentage using the consensus parameter, but I don't quite know how to achieve that? Granted, there are other parts of the code where consensus constants are hard coded, but if possible, it seems sensible to minimize hard coding them.

3. This patch essentially breaks every test where a data request is involved. I started changing some of them, but in total there are probably a couple hundred of tests that require changing, so I first wanted to check what the best way forward is? The only thing I currently changed in this patch is making sure the test code correctly compiles.

[tmpolaczyk]

Hi, so we haven't made any changes to the ConsensusConstants struct since before mainnet, so I'm not sure how would that work. Most probably we can use TAPI to change it, and then if the magic number changes and nodes are not able to connect to the mainnet, we should be able to find some workaround.

Could we just make this a non-consensus, but fixed, parameter somewhere?

We could, but the main benefit of having it in ConsensusConstants is that we can easily launch a testnet with a different parameter just by changing one line in the witnet.toml, so we should try to keep any configurable parameters there.

Regarding the TransactionsPool::default, you cannot use the consensus constants to set the default value because consensus constants are loaded at runtime from the witnet.toml file. A simple workaround is to do the same as the set_minimum_vtt_fee or set_total_weight_limit functions, which set the value after creating the TransactionsPool. There shouldn't be any problem with setting the default value to 0%, because transactions are only inserted in the TransactionsPool after being validated, so any transactions that have less than 1% reward percentage will not be inserted.

Regarding tests I think most of them will fail because the collateral and value fields are set to dummy values, so the fix is to increase the value. Maybe you can set REQUIRED_REWARD_COLLATERAL_PERCENTAGE = 0 to ensure that old tests keep working, and then create new tests where that value is set to 1%.

The minimum_reward_collateral_percentage config variable looks useful and it is not a breaking change, so if you want you can open another pull request with only that, and it will be more likely to be merged.

[drcpu-github]

Hi, so we haven't made any changes to the ConsensusConstants struct since before mainnet, so I'm not sure how would that work. Most probably we can use TAPI to change it, and then if the magic number changes and nodes are not able to connect to the mainnet, we should be able to find some workaround.

Yes, I was also thinking that TAPI may work for this. The magic number that is broadcasted and checked could probably be guarded in some is_wip_active check.

Regarding the TransactionsPool::default, you cannot use the consensus constants to set the default value because consensus constants are loaded at runtime from the witnet.toml file. A simple workaround is to do the same as the set_minimum_vtt_fee or set_total_weight_limit functions, which set the value after creating the TransactionsPool. There shouldn't be any problem with setting the default value to 0%, because transactions are only inserted in the TransactionsPool after being validated, so any transactions that have less than 1% reward percentage will not be inserted.

I assumed that was the solution and it's basically how I implemented it, albeit at 1%. I was just wondering if it could be set by ConsensusConstants because that felt cleaner.

Regarding tests I think most of them will fail because the collateral and value fields are set to dummy values, so the fix is to increase the value. Maybe you can set REQUIRED_REWARD_COLLATERAL_PERCENTAGE = 0 to ensure that old tests keep working, and then create new tests where that value is set to 1%.

Yeah, basically all tests initially fail because transactions don't get inserted in the TransactionsPool or because the data request is not valid or ... I could of course delete the line in set_minimum_reward_collateral_percentage that checks versus the ConsensusConstant and use the old values by calling that function with a 0 value. Not sure yet. But the way forward is likely indeed to somehow make sure the old tests are still valid rather than modifying all of them.

The minimum_reward_collateral_percentage config variable looks useful and it is not a breaking change, so if you want you can open another pull request with only that, and it will be more likely to be merged.

It is certainly possible to do this in a separate patch. However, for your average miner it does not make sense to have a non-zero minimum_reward_collateral_percentage value as that will result in dropped data requests and losing reputation if others do not also use a non-zero setting. Furthermore, if the majority of miners set this to a non-zero value, the existence of this setting could be abused. The complete patch and the introduction of a required_reward_collateral_percentage offers a way to establish a baseline reward while also minimizing abuse as per the initial discussion. I don't think it makes sense to split it up.

[drcpu-github]

Implemented WIP0022-guards around the relevant code.

Made sure that all tests pass when the ratio is not activated by setting the accepted ratio to u64::MAX.

All tests can be run with WIP0022 activated by uncommenting the active_wips insertions at lines 51 and 65 in mainnet_validations.rs.

I am not quite sure how I can modify the get_magic function in chain.rs to ignore the required_reward_collateral_ratio field until after WIP0022 is activated. The best option I can think of right now is to hard-code the current magic number in an if-else construct which seems rather dirty.

[tmpolaczyk]

I rebased this PR and fixed the missing parts here: #2337

Let me know if I missed anything, closing.