fixDAA; implement wtema #3477

tromp · 2020-10-17T13:19:23Z

name: [WIP] fixDAA
about: implement simpler wtema difficulty adjustment algorithm
title: fixDAA
labels: consensus breaking
assignees: antiochp, quentinlesceller, joltz

addresses mimblewimble/grin-rfcs#61

antiochp · 2020-10-18T11:14:41Z

Posted a question related to wtema and FlyClient in the DAA RFC -
mimblewimble/grin-rfcs#61 (comment)

antiochp · 2020-10-19T14:44:55Z

core/src/core/block.rs

-			> Utc::now() + Duration::seconds(12 * (consensus::BLOCK_TIME_SEC as i64))
-		{
-			// refuse blocks more than 12 blocks intervals in future (as in bitcoin)
+		if header.timestamp > Utc::now() + Duration::seconds(5 * 60) {


@tromp this is what you were asking in keybase about?

We do not have access to config directly here. But we could potentially expose it via lazy_static! and thread_local! similar to what we do for CHAIN_TYPE and NRD_FEATURE_ENABLED.

That way we could have something like global::get_future_time_limit() following the same pattern as global::get_chain_type().

The reason for introducing the thread_local! complexity is to make this configurable in tests. Otherwise we have a single global config value that ends up causing havoc across multiple tests.

I'm not sure this is something we would want to test anytime soon.
Is implementation much simpler if we forego testing different settings?

Not really. We're better off just following the existing pattern I think.
Or just leaving it as hardcoded for now. Maybe just move the constant to global.rs and reference it from there.

Following existing pattern now.

core/src/core/block.rs

antiochp · 2020-10-29T17:13:27Z

src/bin/grin.rs

@@ -148,7 +148,7 @@ fn real_main() -> i32 {

 	// Initialize our global chain_type and feature flags (NRD kernel support currently).
 	// These are read via global and not read from config beyond this point.
-	global::init_global_chain_type(config.members.unwrap().server.chain_type);
+	global::init_global_chain_type(config.members.as_mut().unwrap().server.chain_type);


as_mut() not required here?

my understanding is that all but the last access to config.members needs to have as_mut(),
as suggested by lines 127 and 128.

Yeah - this whole init of the config needs rethinking in grin.rs. It looks like we have repeatedly just hacked this until it worked...

We should not need to have mut config here as we do not want to inadvertently write to it. But it needs to be mut because something else higher up in the file has modified a default via a mut on some other piece of nested config.

👍 on getting this working for now - but we should revisit at some point.

i see that grin-wallet uses

global::init_global_chain_type( config .members .as_ref() .unwrap() .wallet .chain_type .as_ref() .unwrap() .clone(), );

perhaps that's better than as_mut() ?

do you agree with adding a global::init_global_accept_fee_base() method and associated
global::get_accept_fee_base() for the benefit of grin-wallet?
It would call global::init_global_accept_fee_base() after parsing the grin-wallet.toml file,
and then pass global::get_accept_fee_base() as 4th argument whenever calling txlib::mod::tx_fee

I decided to have an accept_fee_base parameter in BOTH grin-server.toml and grin-wallet.toml,
and both using global::get_accept_fee_base() to query its value. It's even possible (but not particularly useful) to set then to different values in simultaneously running server and wallet.

As a result, we can simply libtx::mod::tx_fee to no longer have an optional fee base argument. If you want to deviate from the default, you can just change it locally, like I do in some tests.

antiochp · 2020-10-29T17:16:44Z

core/src/global.rs

+				ftl
+			} else {
+				// Global config unset, default to 5 minutes
+				5 * 60


We should call set_local_future_time_limit() either way here so we just read local config next time.
This way we are simply lazily initializing the local config.

Does that apply to is_nrd_enabled() as well?

set_local_future_time_limit() always called now

antiochp · 2020-10-29T17:18:46Z

servers/src/common/types.rs

@@ -214,6 +217,7 @@ impl Default for ServerConfig {
 			dandelion_config: pool::DandelionConfig::default(),
 			stratum_mining_config: Some(StratumServerConfig::default()),
 			chain_type: ChainTypes::default(),
+			future_time_limit: 5 * 60,


We should make 5 * 60 a const in global.rs and reuse it in get_future_time_limit() above.

Sounds good; but then we should also use config::API_SECRET_FILE_NAME rather than ".api_secret" just above?

default now defined as global::DEFAULT_FUTURE_TIME_LIMIT

antiochp · 2020-10-29T17:54:56Z

Came across this and trying to wrap my head around some of it -
zawy12/difficulty-algorithms#30

POW consensus must:
...
2. Have a future timestamp limit that is > 2x "expected" node time error & << difficulty averaging window (strictly speaking, << block time).

Is (2) here relevant to our decision to use a 5 minute FTL?

tromp · 2020-10-30T10:10:56Z

Is (2) here relevant to our decision to use a 5 minute FTL?

zawy argues that an FTL shorter than the blocktime is the best guard against timestamp manipulation. Which is undeniably true; but other people think that doing so carries too much risk of having the local clocks move out of sync beyond the FTL. So our choice of 5 minutes is a compromise that allows some but not much timestamp manipulation, while keeping the risk of out of sync local time very small (the idea being that a clock off by 5 minutes is quite human noticeable, while 1 minute is not so much).

antiochp

Looks good to me.
I did not review the actual next_wtema_difficulty() implementation.

antiochp · 2020-11-18T12:48:15Z

core/tests/consensus_automated.rs

@@ -148,7 +215,7 @@ fn repeat(interval: u64, diff: HeaderInfo, len: u64, cur_time: Option<u64>) -> V
 		.collect::<Vec<_>>()
 }

-fn repeat_offs(from: u64, interval: u64, diff: u64, len: u64) -> Vec<HeaderInfo> {
+fn repeat_offs(interval: u64, diff: u64, len: u64, from: u64) -> Vec<HeaderInfo> {


Glad this is not called many times and we don't need to review those params getting juggled around... 😅

lehnberg · 2020-11-20T15:25:52Z

fixes: #3473

antiochp · 2020-11-24T16:48:38Z

@quentinlesceller @j01tz I added you as reviewers (@tromp has you listed in the PR description).

j01tz · 2020-11-24T18:55:45Z

core/src/consensus.rs

+pub const DMA_WINDOW: u64 = HOUR_HEIGHT;
+
+/// Difficulty adjustment half life is 4 hours
+pub const WTEMA_HALF_LIFE: u64 = 4 * HOUR_SEC;


How was WTEMA_HALF_LIFE parameter selection determined?

it's a tradeoff between response speed and stability. the 240 blocks closely matches BTH's choice of the equivalent of 288 blocks.

j01tz · 2020-11-24T19:29:52Z

core/src/consensus.rs

+	let last_diff = last_header.difficulty.to_num();
+
+	// wtema difficulty update
+	let next_diff =


I'm struggling a bit verifying this algorithm against the details in the RFC and WTEMA specification. It seems straightforward enough here, but I think I need to better wrap my head around the WTEMA_HALF_LIFE value. Maybe a line or two of documentation could be helpful here.

jaspervdm

Looks good! 👍

jaspervdm · 2020-11-24T19:19:36Z

doc/pow/pow.md

@@ -1,43 +1,34 @@
-# Grin's Proof-of-Work


I think the changes in this doc are not relevant to the difficulty adjustment algorithm. Maybe we should have them in a separate PR instead?

the doc fixes don't need review so this seemed the easiest way to get then applied.

jaspervdm · 2020-11-24T19:42:39Z

core/src/consensus.rs

+
+	// wtema difficulty update
+	let next_diff =
+		last_diff * WTEMA_HALF_LIFE / (WTEMA_HALF_LIFE - BLOCK_TIME_SEC + last_block_time);


Took me a bit to process so putting a note here: in terms of the RFC, WTEMA_HALF_LIFE corresponds to W * T. So in this PR we have a W of 240, meaning the difficulty can change by a factor ~e every 240 blocks (4 hours, hence the name of WTEMA_HALF_LIFE) 👍

correct. the name HALF_LIFE is a bit misleading in that sense, since it corresponds not to a doubling.
but that's the terminology that jtoomim and zawy seem to use.

tromp added 2 commits October 17, 2020 15:04

implement wtema

e8d9cc4

reduce FTL from 12 to 5 mins; fix inaccuracies in cuckoo doc

40481df

rename difficulty window for DampedMovingAverage DAA

8ec8b4d

antiochp reviewed Oct 19, 2020

View reviewed changes

core/src/core/block.rs Outdated Show resolved Hide resolved

tromp added 4 commits October 19, 2020 23:48

fix FTL error msg

e8f5642

secondary_scale becomes extended nonce past HF4

449b7db

secondary_scale change actually belongs in hardfork4 PR

4f1c061

make future time limit configurable

d348a5e

antiochp reviewed Oct 29, 2020

View reviewed changes

tromp added 5 commits November 2, 2020 17:57

define constant global::DEFAULT_FUTURE_TIME_LIMIT and refactor a bit

6e094a2

fix typos

c9f7d6f

as_ref() feels safer than as_mut() :-)

bd90143

remove obsolete mut

6c8f535

make future_time_limit non-mandatory in grin-server.toml

2535a75

antiochp approved these changes Nov 18, 2020

View reviewed changes

DavidBurkett mentioned this pull request Nov 19, 2020

Hardfork4 Consensus Changes GrinPlusPlus/GrinPlusPlus#157

Closed

tromp changed the title ~~[WIP] fixDAA; implement wtema~~ fixDAA; implement wtema Nov 23, 2020

Merge branch 'master' into fixdaa

97e2f38

antiochp requested review from quentinlesceller and j01tz November 24, 2020 16:47

j01tz reviewed Nov 24, 2020

View reviewed changes

jaspervdm approved these changes Nov 24, 2020

View reviewed changes

j01tz approved these changes Nov 24, 2020

View reviewed changes

antiochp merged commit f86102b into mimblewimble:master Nov 25, 2020

This was referenced Nov 25, 2020

Tracking issue: Fix DAA RFC #3473

Closed

fix calls to next_diff() in tests to use next height correctly #3501

Merged

v5.0.0 Release Notes #3506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixDAA; implement wtema #3477

fixDAA; implement wtema #3477

tromp commented Oct 17, 2020 •

edited

Loading

antiochp commented Oct 18, 2020

antiochp Oct 19, 2020

tromp Oct 19, 2020

antiochp Oct 22, 2020

tromp Nov 3, 2020

antiochp Oct 29, 2020

tromp Oct 29, 2020

antiochp Oct 29, 2020

antiochp Oct 29, 2020

tromp Oct 29, 2020

tromp Oct 29, 2020

tromp Nov 2, 2020

antiochp Oct 29, 2020

tromp Oct 29, 2020

tromp Nov 3, 2020

antiochp Oct 29, 2020

tromp Oct 29, 2020

tromp Nov 3, 2020

antiochp commented Oct 29, 2020 •

edited

Loading

tromp commented Oct 30, 2020

antiochp left a comment •

edited

Loading

antiochp Nov 18, 2020

lehnberg commented Nov 20, 2020

antiochp commented Nov 24, 2020

j01tz Nov 24, 2020

tromp Nov 24, 2020

j01tz Nov 24, 2020

jaspervdm left a comment

jaspervdm Nov 24, 2020

tromp Nov 24, 2020

jaspervdm Nov 24, 2020

tromp Nov 24, 2020 •

edited

Loading

fixDAA; implement wtema #3477

fixDAA; implement wtema #3477

Conversation

tromp commented Oct 17, 2020 • edited Loading

antiochp commented Oct 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antiochp commented Oct 29, 2020 • edited Loading

tromp commented Oct 30, 2020

antiochp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lehnberg commented Nov 20, 2020

antiochp commented Nov 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaspervdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromp Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

tromp commented Oct 17, 2020 •

edited

Loading

antiochp commented Oct 29, 2020 •

edited

Loading

antiochp left a comment •

edited

Loading

tromp Nov 24, 2020 •

edited

Loading