feat(consensus): Change meta.score and meta.accumulated_weight to int type #572

msbrogli · 2023-04-27T02:26:33Z

Motivation

The metadata accumulated_weight and score are calculated summing up the vertices' weight. Even though both metadata can be integer numbers, they are calculated as the logarithm of their integer value. This might lead to minor issues caused by precision error. We currently use the calculate_min_significant_weight() to prevent precision error on mining activity.

This PR converts both metadata to integer values, simplifying parts of the code that had to take float precision into consideration.

Acceptance criteria

Change meta.score and meta.accumulated_weight to type int.
Change the calculation of meta.score to use weight_to_work().
Change the calculation of meta.accumulated_weight to use weight_to_work().
Change the consensus algorithm to do direct comparisons (instead of comparisons using WEIGHT_TOL).
Replace sum_weights(a, b) to weight_to_work(a) + weight_to_work(b).
Replace sum_weights(score, a) to score + weight_to_work(a).
Replace sum_weights(accumulated_weight, a) to accumulated_weight + weight_to_work(a).
Remove sum_weights().
Serialize meta.score and meta.accumulated_weight as strings to prevent precision issues with JSON parsers.

codecov · 2023-12-21T20:06:34Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ded4c79) 85.30% compared to head (57d882b) 85.36%.

❗ Current head 57d882b differs from pull request most recent head 795c414. Consider uploading reports for the commit 795c414 to get more accurate results

Files	Patch %	Lines
hathor/consensus/block_consensus.py	84.21%	1 Missing and 2 partials ⚠️
...age/migrations/change_score_acc_weight_metadata.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   85.30%   85.36%   +0.05%     
==========================================
  Files         285      284       -1     
  Lines       22365    22310      -55     
  Branches     3369     3362       -7     
==========================================
- Hits        19078    19044      -34     
+ Misses       2612     2593      -19     
+ Partials      675      673       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jansegre · 2024-01-11T18:01:21Z

I haven't reviewed the code yet, but I think we should keep calculate_min_significant_weight for at least one release, in order to prevent an updated miner to propagate a block that would cause an issue with an outdated node.

hathor/transaction/resources/transaction.py

hathor/transaction/storage/migrations/change_score_acc_weight_metadata.py

hathor/transaction/base_transaction.py

hathor/consensus/block_consensus.py

jansegre · 2024-01-16T16:56:47Z

hathor/manager.py

-        # this is the min weight to cause an increase of twice the WEIGHT_TOL, we make sure to generate a template with
-        # at least this weight (note that the user of the API can set its own weight, the block sumit API will also
-        # protect agains a weight that is too small but using WEIGHT_TOL instead of 2*WEIGHT_TOL)
-        min_significant_weight = calculate_min_significant_weight(
-            parent_block_metadata.score,
-            2 * self._settings.WEIGHT_TOL
-        )
-        weight = max(self.daa.calculate_next_weight(parent_block, timestamp), min_significant_weight)
+        weight = self.daa.calculate_next_weight(parent_block, timestamp)


Keep calculate_min_significant_weight for at least one more release. Maybe something like TODO: remove calculate_min_significant_weight in the future.

jansegre · 2024-01-16T16:57:23Z

hathor/manager.py

-        min_insignificant_weight = calculate_min_significant_weight(
-            parent_block_metadata.score,
-            self._settings.WEIGHT_TOL
-        )
-        if blk.weight <= min_insignificant_weight:
-            self.log.warn('submit_block(): insignificant weight? accepted anyway', blk=blk.hash_hex, weight=blk.weight)


Keep calculate_min_significant_weight for at least one more release. Maybe something like TODO: remove calculate_min_significant_weight in the future.

msbrogli · 2024-01-19T06:31:47Z

hathor/utils/weight.py

+def weight_to_work(weight: float) -> int:
+    """Convert weight to work rounding to the nearest integer."""
+    # return int(0.5 + 2**weight)
+    return int(0.5 + exp(weight * _LN_2))


Which one has greater precision?

I've made some tests comparing the two methods against Wolphram Alpha (as a quick with high precision), using this type of query: https://www.wolframalpha.com/input?i=Ceil%5B2%5E88.50000000000001%5D

method 1: int(0.5 + 2**weight)
method 2: int(0.5 + exp(weight * _LN_2))
(I also tested math.exp2 and it's exactly the same as 2**weight)

These are some results for weights that we can expect:

weight method 1 method 2 wolphram

55 36028797018963968 36028797018963824 36028797018963968

55.5 50952413380206184 50952413380206024 50952413380206181

60 1152921504606846976 1152921504606844800 1152921504606846976

60.001 1153720925923479040 1153720925923474176 1153720925923480963

60.00001 1152929496077448960 1152929496077448192 1152929496077446523

88 309485009821345068724781056 309485009821345274883211264 309485009821345068724781056

88.5 437677898240516748033916928 437677898240514274132754432 437677898240516718115093268

88.50000000000001 437677898240521008641474560 437677898240520527605137408 437677898240519751867104856

And the errors:

weight method 1 (err) method 2 (err)

55 0 144

55.5 3 157

60 0 2176

60.001 1923 6787

60.00001 2437 1669

88 0 206158430208

88.5 29918823660 2443982338836

88.50000000000001 1256774369704 775738032552

It doesn't look too conclusive, I think method 1 will favor integer values of weight, but method 2 might favor some other values. I wonder if there's any better method to calculate it while being efficient. I don't think it would be worth to use a multi precision math lib though.

msbrogli · 2024-01-19T06:36:40Z

I haven't reviewed the code yet, but I think we should keep calculate_min_significant_weight for at least one release, in order to prevent an updated miner to propagate a block that would cause an issue with an outdated node.

Nice catch! I just did it. Thanks!

glevco · 2024-01-19T15:45:19Z

hathor/consensus/block_consensus.py

@@ -200,7 +201,7 @@ def update_voided_info(self, block: Block) -> None:
                common_block = self._find_first_parent_in_best_chain(block)
                self.add_voided_by_to_multiple_chains(block, heads, common_block)

-                if score >= best_score + self._settings.WEIGHT_TOL:
+                if score > best_score:


Why was the = removed?

Here we are checking if the block has the highest score. So we must check if score > best_score.

@glevco and @jansegre , confirm that you agree that I did the right thing. I'll wait for your confirmation before resolving this conversation.

hathor/consensus/block_consensus.py

… type

msbrogli · 2024-01-19T19:23:01Z

hathor/transaction/base_transaction.py

@@ -60,26 +61,6 @@
 _ONE_BYTE = 0xFF


-def sum_weights(w1: float, w2: float) -> float:


I think I won't be able to remove the sum_weights() in this PR because it is still used in 3 places:

metrics.py: Seems easy to remove it.

difficulty.py: This is used by merged mining and maybe Jan can help me understand the consequences of changing it. Anyway, it seems feasible.

stratum/straum.py: Seems easy but we need to better assess its impact. I think it's used in the hash rate estimator only. And, to be honest, I don't know if we still use this code.

daa.py: I think we should not change this one, as it might affect the minimum weight of the blocks and invalidate blocks that are on the mainnet. To confirm the consequence, we would need to check it against the mainnet but it seems not worth the risk.

My impression is that, in the end, we will remove the sum_weights() from (1), (2), and (3) and then we will move its code to the daa.py and continue using it there.

What do you think?

Makes sense to me.

In difficulty it is only used to implement a logsum for function reuse. The following patch ports the exact same math:

diff --git a/hathor/difficulty.py b/hathor/difficulty.py index 4a8467ef..fa80c124 100644 --- a/hathor/difficulty.py +++ b/hathor/difficulty.py @@ -787,8 +787,8 @@ class Weight(float): Currently is just a proxy to `hathor.transaction.sum_weights`. """ - from hathor.transaction import sum_weights - return Weight(sum_weights(self, other)) + a, b = self, other if self > other else other, self + return Weight(a + log2(1 + 2 ** (b - a))) def to_u256(self) -> U256: """ Convert to U256

Since hathor.difficulty is intended to abstract that kind of values, it can be used in places like hathor.metrics, hathor.stratum.stratum and hathor.daa. That should preserve the exact same operations as before. I can see how hathor.daa could maybe be refactored but I agree it's not worth to touch it now.

glevco · 2024-01-19T19:11:38Z

hathor/consensus/block_consensus.py

-                    assert abs(best_score - head_meta.score) < 1e-10
-            assert isinstance(best_score, (int, float))
+                    assert best_score == head_meta.score
+            assert isinstance(best_score, int)


Maybe set the type of best_score: int | None = None above, and then this could be assert best_score is not None

hathor/consensus/block_consensus.py

glevco · 2024-01-19T19:28:54Z

hathor/consensus/block_consensus.py

@@ -200,7 +201,7 @@ def update_voided_info(self, block: Block) -> None:
                common_block = self._find_first_parent_in_best_chain(block)
                self.add_voided_by_to_multiple_chains(block, heads, common_block)

-                if score >= best_score + self._settings.WEIGHT_TOL:
+                if score > best_score:


hathor/consensus/block_consensus.py

glevco · 2024-01-19T20:05:20Z

hathor/manager.py

We have to update the calls to calculate_min_significant_weight(), or rather, the function itself, right? It's expecting to receive a log_score, but we're passing the new score.

hathor/consensus/block_consensus.py

jansegre · 2024-01-19T19:43:08Z

hathor/transaction/storage/migrations/change_score_acc_weight_metadata.py

+    def run(self, storage: 'TransactionStorage') -> None:
+        raise Exception('Cannot migrate your database due to an incompatible change in the metadata. '
+                        'Please, delete your data folder and use the latest available snapshot or sync '
+                        'from beginning.')


This migration implies we can remove all the other migrations. I think we should do that in another PR before a release.

I agree, and I have two open PRs (#927, #931) that have migrations. It would be good if we could merge them before this one.

I just released #933 which also has a migration, so it would be nice to wait for it too.

jansegre · 2024-01-19T21:15:08Z

hathor/utils/weight.py

+def weight_to_work(weight: float) -> int:
+    """Convert weight to work rounding to the nearest integer."""
+    # return int(0.5 + 2**weight)
+    return int(0.5 + exp(weight * _LN_2))


I've made some tests comparing the two methods against Wolphram Alpha (as a quick with high precision), using this type of query: https://www.wolframalpha.com/input?i=Ceil%5B2%5E88.50000000000001%5D

method 1: int(0.5 + 2**weight)
method 2: int(0.5 + exp(weight * _LN_2))
(I also tested math.exp2 and it's exactly the same as 2**weight)

These are some results for weights that we can expect:

weight method 1 method 2 wolphram

55 36028797018963968 36028797018963824 36028797018963968

55.5 50952413380206184 50952413380206024 50952413380206181

60 1152921504606846976 1152921504606844800 1152921504606846976

60.001 1153720925923479040 1153720925923474176 1153720925923480963

60.00001 1152929496077448960 1152929496077448192 1152929496077446523

88 309485009821345068724781056 309485009821345274883211264 309485009821345068724781056

88.5 437677898240516748033916928 437677898240514274132754432 437677898240516718115093268

88.50000000000001 437677898240521008641474560 437677898240520527605137408 437677898240519751867104856

And the errors:

weight method 1 (err) method 2 (err)

55 0 144

55.5 3 157

60 0 2176

60.001 1923 6787

60.00001 2437 1669

88 0 206158430208

88.5 29918823660 2443982338836

88.50000000000001 1256774369704 775738032552

It doesn't look too conclusive, I think method 1 will favor integer values of weight, but method 2 might favor some other values. I wonder if there's any better method to calculate it while being efficient. I don't think it would be worth to use a multi precision math lib though.

msbrogli added the enhancement New feature or request label Apr 27, 2023

msbrogli requested a review from jansegre as a code owner April 27, 2023 02:26

msbrogli self-assigned this Apr 27, 2023

msbrogli force-pushed the feat/score-acc-weight-types branch 4 times, most recently from 265afc8 to 65ccfdd Compare April 27, 2023 04:49

msbrogli changed the base branch from master to refactor/consensus-multiple-files April 27, 2023 04:49

msbrogli force-pushed the feat/score-acc-weight-types branch from 65ccfdd to 21acfdb Compare April 27, 2023 05:03

msbrogli force-pushed the refactor/consensus-multiple-files branch from 69098cf to fd2bf10 Compare April 27, 2023 05:04

msbrogli force-pushed the feat/score-acc-weight-types branch 5 times, most recently from 1b625ad to 12084a4 Compare April 27, 2023 17:56

msbrogli force-pushed the refactor/consensus-multiple-files branch 2 times, most recently from 0bf14d9 to 86ce84c Compare April 27, 2023 18:08

Base automatically changed from refactor/consensus-multiple-files to master April 27, 2023 18:41

msbrogli force-pushed the feat/score-acc-weight-types branch 5 times, most recently from a48df48 to 83760be Compare April 27, 2023 21:06

msbrogli force-pushed the feat/score-acc-weight-types branch 3 times, most recently from 289dc7f to b0d8f5c Compare June 13, 2023 18:58

msbrogli force-pushed the feat/score-acc-weight-types branch from b0d8f5c to 81b60c9 Compare June 15, 2023 15:30

msbrogli force-pushed the feat/score-acc-weight-types branch 3 times, most recently from 8e4ecfe to 464fb5b Compare November 11, 2023 06:08

msbrogli force-pushed the feat/score-acc-weight-types branch 4 times, most recently from 81a5b71 to d2e5e2a Compare December 21, 2023 00:59

msbrogli force-pushed the feat/score-acc-weight-types branch 2 times, most recently from 60c7fa2 to edc25af Compare January 9, 2024 17:06

msbrogli requested a review from glevco January 10, 2024 16:27

glevco requested changes Jan 15, 2024

View reviewed changes

jansegre reviewed Jan 16, 2024

View reviewed changes

msbrogli force-pushed the feat/score-acc-weight-types branch from edc25af to 26376fb Compare January 19, 2024 06:30

msbrogli commented Jan 19, 2024

View reviewed changes

msbrogli force-pushed the feat/score-acc-weight-types branch 2 times, most recently from 50a01de to 5322ec4 Compare January 19, 2024 06:42

glevco reviewed Jan 19, 2024

View reviewed changes

msbrogli force-pushed the feat/score-acc-weight-types branch 2 times, most recently from 62e02ee to 83a6cfd Compare January 19, 2024 18:54

msbrogli added 3 commits January 19, 2024 12:54

feat(consensus): Change meta.score and meta.accumulated_weight to int…

4ed316e

… type

review changes

242be2f

small changes

795c414

msbrogli force-pushed the feat/score-acc-weight-types branch from 83a6cfd to 795c414 Compare January 19, 2024 18:55

msbrogli commented Jan 19, 2024

View reviewed changes

glevco reviewed Jan 19, 2024

View reviewed changes

jansegre approved these changes Jan 23, 2024

View reviewed changes

jansegre mentioned this pull request Jan 23, 2024

chore(feature-activation): phased testing adjustments #931

Merged

1 task

glevco mentioned this pull request Jan 26, 2024

fix(feature-activation): add missing metadata migration #927

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(consensus): Change meta.score and meta.accumulated_weight to int type #572

feat(consensus): Change meta.score and meta.accumulated_weight to int type #572

msbrogli commented Apr 27, 2023 •

edited

codecov bot commented Dec 21, 2023 •

edited

jansegre commented Jan 11, 2024

jansegre Jan 16, 2024

msbrogli Jan 19, 2024 •

edited

jansegre Jan 16, 2024

msbrogli Jan 19, 2024

msbrogli Jan 19, 2024

jansegre Jan 19, 2024

msbrogli commented Jan 19, 2024

glevco Jan 19, 2024

msbrogli Jan 19, 2024

glevco Jan 19, 2024

msbrogli Jan 19, 2024

glevco Jan 19, 2024

jansegre Jan 19, 2024

glevco Jan 19, 2024

glevco Jan 19, 2024

glevco Jan 19, 2024

jansegre Jan 19, 2024

glevco Jan 23, 2024

glevco Jan 23, 2024

jansegre Jan 19, 2024

weight	method 1	method 2	wolphram
55	36028797018963968	36028797018963824	36028797018963968
55.5	50952413380206184	50952413380206024	50952413380206181
60	1152921504606846976	1152921504606844800	1152921504606846976
60.001	1153720925923479040	1153720925923474176	1153720925923480963
60.00001	1152929496077448960	1152929496077448192	1152929496077446523
88	309485009821345068724781056	309485009821345274883211264	309485009821345068724781056
88.5	437677898240516748033916928	437677898240514274132754432	437677898240516718115093268
88.50000000000001	437677898240521008641474560	437677898240520527605137408	437677898240519751867104856

weight	method 1 (err)	method 2 (err)
55	0	144
55.5	3	157
60	0	2176
60.001	1923	6787
60.00001	2437	1669
88	0	206158430208
88.5	29918823660	2443982338836
88.50000000000001	1256774369704	775738032552

		@@ -60,26 +61,6 @@
		_ONE_BYTE = 0xFF


		def sum_weights(w1: float, w2: float) -> float:

feat(consensus): Change meta.score and meta.accumulated_weight to int type #572

Are you sure you want to change the base?

feat(consensus): Change meta.score and meta.accumulated_weight to int type #572

Conversation

msbrogli commented Apr 27, 2023 • edited

Motivation

Acceptance criteria

codecov bot commented Dec 21, 2023 • edited

Codecov Report

jansegre commented Jan 11, 2024

Choose a reason for hiding this comment

msbrogli Jan 19, 2024 • edited

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

msbrogli commented Jan 19, 2024

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

msbrogli commented Apr 27, 2023 •

edited

codecov bot commented Dec 21, 2023 •

edited

msbrogli Jan 19, 2024 •

edited