Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix the broken Weight Multiplier #6328

Closed
2 tasks done
kianenigma opened this issue Jun 11, 2020 · 0 comments · Fixed by #6334
Closed
2 tasks done

Fix the broken Weight Multiplier #6328

kianenigma opened this issue Jun 11, 2020 · 0 comments · Fixed by #6334
Labels
I3-bug The node fails to follow expected behavior. U2-some_time_soon Issue is worth doing soon.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented Jun 11, 2020

The current weight multiplier implementation is unfortunately wrong. From the research page we have:

ctraffic ← ctraffic ⋅ ( 1 + v(s−ss) + v^2 (s−ss)^2 / 2).

Assuming:

t1 = v(s−ss)
t2 = v^2 (s−ss)^2 / 2

This is in short:

next = prev + prev.mul(t1 + t2)

While our implementation does:

if positive {
// Note: this is merely bounded by how big the multiplier and the inner value can go,
// not by any economical reasoning.
let excess = first_term.saturating_add(second_term);
multiplier.saturating_add(excess)
} else {
// Defensive-only: first_term > second_term. Safe subtraction.
let negative = first_term.saturating_sub(second_term);
multiplier.saturating_sub(negative)
// despite the fact that apply_to saturates weight (final fee cannot go below 0)
// it is crucially important to stop here and don't further reduce the weight fee
// multiplier. While at -1, it means that the network is so un-congested that all
// transactions have no weight fee. We stop here and only increase if the network
// became more busy.
.max(Multiplier::saturating_from_integer(-1))

Which in short is

next = prev + (t1 + t2)

Then the gist of the fix will be:

if positive {
	// Note: this is merely bounded by how big the previous and the inner value can go,
	// not by any economical reasoning.
+ 	 let excess = first_term.saturating_add(second_term).saturating_mul(previous);
- 	 let excess = first_term.saturating_add(second_term);
	previous.saturating_add(excess)
} else {
	// Defensive-only: first_term > second_term. Safe subtraction.
+ 	let negative = first_term.saturating_sub(second_term).saturating_mul(previous);
- 	let negative = first_term.saturating_sub(second_term);
	previous.saturating_sub(negative)
		// despite the fact that apply_to saturates weight (final fee cannot go below 0)
		// it is crucially important to stop here and don't further reduce the weight fee
		// previous. While at -1, it means that the network is so un-congested that all
		// transactions have no weight fee. We stop here and only increase if the network
		// became more busy.
		.max(Multiplier::saturating_from_integer(-1))
}

And fixing a number of tests. I already started with this and there's a problem: Given the default parameters here, if we let multiplier go down to 0, then it will never recover, and it is unclear how to do this.

  • Moreover, the research says that the ratio should be computed only from the normal transaction types, with respect to the normal limit. We don't do this now and this needs fixing as well.
  • When applying the multiplier, we apply this +1 (multiply_accumulate) jumbo mumbo that is quite unnecessary in my opinion given the fix.
@kianenigma kianenigma added I3-bug The node fails to follow expected behavior. U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Jun 11, 2020
@kianenigma kianenigma changed the title Fix the broken Weight Mulriplier Fix the broken Weight Multiplier Jun 11, 2020
@kianenigma kianenigma added U2-some_time_soon Issue is worth doing soon. and removed U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Jun 16, 2020
@ghost ghost closed this as completed in #6334 Jun 17, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I3-bug The node fails to follow expected behavior. U2-some_time_soon Issue is worth doing soon.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant