feat: inflating tail #6131
feat: inflating tail #6131
Conversation
ghpbot-tari-project
added
P-acks_required
Test Results (CI) 2 files 78 suites 29m 52s ⏱️ For more details on these failures, see this check. Results for commit 7a6d6ac. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)29 tests 29 ✅ 11m 30s ⏱️ Results for commit 7a6d6ac. ♻️ This comment has been updated with latest results. |
CjS77
added
A-base_node
CjS77
force-pushed
the
inflating_tail
branch
4 times, most recently
from
938ab74
to
1f3b1b6
Compare
| build_features(); | ||
| (); |
| // to determine the correct inflation curve. So we need to include the premine in the emission curve. | ||
| let total_supply = self.rules.get_total_emission_at(height) + | ||
| if cfg!(tari_feature_mainnet_emission) { | ||
| MicroMinotari::from(0) |
There was a problem hiding this comment.
this 0 does not seem correct.
Does mainnet emission not have a faucet value?
If you read the PR description, I explain the need and rationale for testing the feature gate before mainnet. This is as good a test as any. |
| @@ -105,23 +105,23 @@ fn print_mr_values(block: &mut Block, print: bool) { | |||
|
|
|||
| pub fn get_stagenet_genesis_block() -> ChainBlock { | |||
| let mut block = get_stagenet_genesis_block_raw(); | |||
|
|
|||
| eprintln!("Network: {:?}", Network::get_current_or_default()); | |||
This PR primarily adds tail emission inflation as a featuer; but it does so using feature gates, which were broken in a few places, and so a big part of this PR is resolving issues related to feature gates. == Tail emission inflation Adds a feature tari_feature_mainnet_emission to allow for tail emission inflation. See #6122 This change necessitates the addition of 2 new consensus constants: `inflation_bips` -- the annual inflation rate of the total supply. and `tail_emission_epoch_length`, which controls the tail emission inflation. These replace `tail_emission`. We update the Protobuf definition for ConsensusConstants to account for the new fields. == Getting Feature flags working When formally implementing compiler features, the setting of the `TARI_NETWORK` and (from now) `TARI_TARGET_NETWORK` envar and the related compiler flags matter. These changes require that when tests run, the correct network flags are set. For example, blocks::genesis_block::test::stagenet_genesis_sanity_check should only run when TARI_NETWORK=stagenet. While writing this PR, I uncovered a problem: There is not a 1:1 mapping between the build _target_ and the actual network the binary may run on. E.g. the mainnet build can run on either stagenet or mainnet. Unfortunately the `TARI_NETWORK` envar was used to set both of these variables. Without using feature flags, this discrepency went unnoticed. With feature flags, a lot of tests that implicitly assumed say a mainnet target and a stagenet run would simply fail, with no way to resolve the issue. Since these are 2 different things, this commit introduced `TARI_TARGET_NETWORK` whicg sets the **build target**. In most contexts, this then decouples the configured network, which is specified by the `TARI_NETWORK` envar as usual. The other changes in this commit revolve around updating merkle roots and the correct faucet sets to make the target/network/faucet combination consistent. == CI workflows The CI is configured to run tests for each of * Testnet (Esme) * Nextnet (Nextnet) * Mainnet (Mainnet, with some tests specifying stagenet) === remove env_logger I used env_logger to try print the feature list at compiler time, but it doesn't work, so rather remove it. A better strategy (implemented in this commit) is to print "tari_feature" as a prefix to the message emitted by build.rs. Then you can do soemthing like `cargo build -vv .... 2>/dev/null` | grep "tari_feature"` Piping stderr to dev null hides all the noise from rustc, and then you can grep for the messages you want (or omit the grep to see the usual build messages among the rest) === set network according to pols Sets the network according to the principle of least surprise. i.e. if TARI_NETWORK is set, try and make that the default network.
There was a problem hiding this comment.
I can appreciate that a lot was accomplished in this PR.
However, I think it would be better to have a tail emission and then inflate the tail emission if inflating tail emission is required. When inflating tail emission is not desired that value can be zero in the consensus constants. This would make the code much simpler and more consistent and remove the requirement to have the tari_feature_mainnet_emission feature flag.
| // Comment out the feature flag to run this test | ||
| #[test] | ||
| #[cfg(feature = "schedule_get_constants")] | ||
| fn esmeralda_schedule_get_constants() { |
There was a problem hiding this comment.
Why is this test removed? It should be added back in.
There was a problem hiding this comment.
It's covered by the new tail emission test. Esme's constants are changing with the tail inflation, so the test would fail anyway.
| } | ||
|
|
||
| #[test] | ||
| fn esmeralda_schedule() { |
There was a problem hiding this comment.
Why is this test removed? It should be added back in.
| } | ||
|
|
||
| #[test] | ||
| fn nextnet_schedule() { |
There was a problem hiding this comment.
Why is this test removed? It should be added back in.
| } | ||
|
|
||
| #[test] | ||
| fn stagenet_schedule() { |
There was a problem hiding this comment.
Why is this test removed? It should be added back in.
| } | ||
|
|
||
| #[test] | ||
| fn igor_schedule() { |
There was a problem hiding this comment.
Why is this test removed?
| pub(in crate::consensus) emission_tail: MicroMinotari, | ||
| /// The tail emission inflation rate in bips |
There was a problem hiding this comment.
| /// The tail emission inflation rate in bips | |
| /// The tail emission inflation rate in bips (basis points, otherwise known as bps or "bips") |
| @@ -167,6 +167,17 @@ jobs: | |||
| permissions: | |||
| checks: write | |||
| pull-requests: write | |||
| strategy: | |||
| matrix: | |||
| tari_target_network: [ | |||
There was a problem hiding this comment.
Maybe a better name choice
| tari_target_network: [ | |
| tari_network_map: [ |
| #[cfg(not(tari_feature_mainnet_emission))] | ||
| let (emission_initial, emission_decay, emission_tail) = cc.emission_amounts(); | ||
| #[cfg(tari_feature_mainnet_emission)] |
There was a problem hiding this comment.
Why have this differentiation at all? It seems more prudent to choose if we will have inflating tail emission or not.
There was a problem hiding this comment.
I'm not sure I understand the question.
If you're asking why inflating tail emission is using a feature gate, I gave a rationale in other comments.
There was a problem hiding this comment.
Why have both implementations in the code if we will only use one?
| @@ -399,11 +393,13 @@ mod test { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn esmeralda_genesis_sanity_check() { | |||
There was a problem hiding this comment.
This esemeralda genesis block sanity test should not be removed, it should be added back in. We need to do the genesis block sanity check for every network.
I'm happy to hear alternatives. What you are suggesting is not 100% clear. Are you suggesting inflating tail emission via hardforks "as we go" every year? If not, and you're proposing a transparent, code-based approach as presented here, then please offer an alternative. Note that your constraints are:
|
|
What I mean is we can implement both schemes without feature flags and without duplicate code. In the event that we want to have tail emission with tail inflation: emission_initial: INITIAL_EMISSION,
emission_decay: &EMISSION_DECAY,
emission_tail: 800 * T,
inflation_bips: 1,
tail_epoch_length: ANNUAL_BLOCKS,In the event that we want to have tail inflation only: emission_initial: INITIAL_EMISSION,
emission_decay: &EMISSION_DECAY,
emission_tail: 0 * T,
inflation_bips: 1,
tail_epoch_length: ANNUAL_BLOCKS,In the event that we want to have tail emission only: emission_initial: INITIAL_EMISSION,
emission_decay: &EMISSION_DECAY,
emission_tail: 800 * T,
inflation_bips: 0,
tail_epoch_length: 0,In the event that we do not want either: emission_initial: INITIAL_EMISSION,
emission_decay: &EMISSION_DECAY,
emission_tail: 0 * T,
inflation_bips: 0,
tail_epoch_length: 0,The fn next_reward(&mut self) {
// Inflation phase
if self.epoch > 0 && self.schedule.inflation_bips > 0 {
self.epoch_counter += 1;
if self.epoch_counter >= self.schedule.epoch_length {
self.epoch_counter = 0;
self.epoch += 1;
self.reward = self.new_tail_emission();
}
} else {
// Decay phase
let cutoff = if self.schedule.tail > MicroMinotari::zero() {
self.schedule.tail
} else {
self.new_tail_emission()
};
let next_decay_reward = self.next_decay_reward();
if self.epoch == 0 && next_decay_reward > cutoff {
self.reward = next_decay_reward;
} else {
self.epoch = 1;
self.reward = cutoff;
}
}
} |
Adds a feature `tari_feature_mainnet_emission` to allow for tail emission inflation. See #6122 This change necessitates the addition of 2 new consensus constants: `inflation_bips` -- the annual inflation rate of the total supply in basis points (100 bips = 1 percentage point). and `tail_emission_epoch_length`, which controls the tail emission inflation. These replace `tail_emission`. We update the Protobuf definition for `ConsensusConstants` to account for the new fields. Updates tests Adds `as_u128` to `MicroMinotari` since we need to perform a large multiplication when calculating the inflation issuance. Note: Replaces part of #6131
When formally implementing compiler features, the setting of the
`TARI_NETWORK` and (from now) `TARI_TARGET_NETWORK` envar and the
related compiler flags matter.
These changes require that when tests run, the correct network flags are
set.
For example,
`blocks::genesis_block::test::stagenet_genesis_sanity_check` should only
run when `TARI_NETWORK=stagenet`.
The current setup reveals a problem:
There is not a 1:1 mapping between the build _target_ and the actual
network the binary may run on. E.g. the mainnet build can run on either
stagenet or mainnet.
Unfortunately the `TARI_NETWORK` envar was used to set both of these
variables.
Since these are 2 different things, this commit introduced
`TARI_TARGET_NETWORK` which sets the **build target**. In most contexts,
this then decouples the configured network, which is specified by the
`TARI_NETWORK` envar as usual.
The other changes in this commit revolve around updating merkle roots
and the correct faucet sets to make the target/network/faucet
combination consistent.
The CI is configured to run tests for each of `{target} ({network})`
* Testnet (Esme)
* Nextnet (Nextnet)
* Mainnet (Stagenet)
Description
---
This replaces part of #6131
Breaking Changes
---
- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify
<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
## Summary Adds a feature `tari_feature_mainnet_emission` to allow for tail emission inflation. See #6122 This change necessitates the addition of 2 new consensus constants: `inflation_bips` -- the annual inflation rate of the total supply. and `tail_emission_epoch_length`, which controls the tail emission inflation. These replace `tail_emission`. We update the Protobuf definition for `ConsensusConstants` to account for the new fields. Note: Replaces part of #6131
Summary
Adds a feature
tari_feature_mainnet_emissionto allow for tail emissioninflation. See #6122
This change necessitates the addition of 2 new consensus constants:
inflation_bips-- the annual inflation rate of the total supply.and
tail_emission_epoch_length, which controls the tail emissioninflation. These replace
tail_emission.We update the Protobuf definition for
ConsensusConstantsto account forthe new fields.
Getting Feature flags working
When formally implementing compiler features, the setting of the
TARI_NETWORKand (from now)
TARI_TARGET_NETWORKenvar and the related compiler flags matter.These changes require that when tests run, the correct network flags are set.
For example,
blocks::genesis_block::test::stagenet_genesis_sanity_checkshouldonly run when
TARI_NETWORK=stagenet.While writing this PR, I uncovered a problem:
There is not a 1:1 mapping between the build target and the actual
network the binary may run on. E.g. the mainnet build can run on either
stagenet or mainnet.
Unfortunately the
TARI_NETWORKenvar was used to set both of thesevariables. Without using feature flags, this discrepancy went unnoticed.
With feature flags, a lot of tests that implicitly assumed say a mainnet
target and a stagenet run would simply fail, with no way to resolve the
issue.
Since these are 2 different things, this commit introduced
TARI_TARGET_NETWORKwhich sets the build target. In most contexts,this then decouples the configured network, which is specified by the
TARI_NETWORKenvar as usual.The other changes in this commit revolve around updating merkle roots
and the correct faucet sets to make the target/network/faucet
combination consistent.
CI workflows
The CI is configured to run tests for each of
{target} ({network})