feat(nano): implement storage module [part 4] by glevco · Pull Request #1279 · HathorNetwork/hathor-core

glevco · 2025-05-30T17:22:19Z

Motivation

Implement the storage module.

Acceptance Criteria

Implement the storage module.

Checklist

If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

codecov · 2025-06-03T02:44:04Z

Codecov Report

Attention: Patch coverage is 39.46449% with 520 lines in your changes missing coverage. Please review.

Project coverage is 81.49%. Comparing base (d0043ba) to head (24fa883).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
hathor/nanocontracts/storage/patricia_trie.py	23.00%	164 Missing ⚠️
hathor/nanocontracts/storage/contract_storage.py	40.56%	126 Missing ⚠️
hathor/nanocontracts/storage/changes_tracker.py	37.73%	99 Missing ⚠️
hathor/nanocontracts/storage/block_storage.py	44.15%	43 Missing ⚠️
hathor/nanocontracts/storage/backends.py	54.38%	26 Missing ⚠️
...thor/nanocontracts/storage/maybedeleted_nc_type.py	44.44%	25 Missing ⚠️
hathor/nanocontracts/storage/node_nc_type.py	47.50%	21 Missing ⚠️
hathor/nanocontracts/storage/factory.py	56.66%	13 Missing ⚠️
hathor/nanocontracts/storage/token_proxy.py	66.66%	3 Missing ⚠️

❌ Your project status has failed because the head coverage (81.49%) is below the target coverage (82.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
- Coverage   82.90%   81.49%   -1.41%     
==========================================
  Files         362      373      +11     
  Lines       25838    26697     +859     
  Branches     3956     4060     +104     
==========================================
+ Hits        21420    21758     +338     
- Misses       3691     4202     +511     
- Partials      727      737      +10

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-06-03T20:05:55Z

Bencher Report

Branch	feat/nano/storages
Testbed	ubuntu-22.04

Click to view all benchmark results

Benchmark	Latency	Benchmark Result minutes (m) (Result Δ%)	Lower Boundary minutes (m) (Limit %)	Upper Boundary minutes (m) (Limit %)
sync-v2 (up to 20000 blocks)	📈 view plot 🚷 view threshold	1.58 m (-2.78%) Baseline: 1.63 m	1.47 m (92.57%)	1.79 m (88.38%)

🐰 View full continuous benchmarking report in Bencher

msbrogli · 2025-06-05T15:56:43Z

hathor/nanocontracts/storage/backends.py

+        raise NotImplementedError
+
+
+class MemoryNodeTrieStore(NodeTrieStore):


Should we remove this memory trie store? Or we should make it serialize and deserialize objects to behave as the RocksDB trie store.

Postponed thread.

hathor/nanocontracts/storage/block_storage.py

msbrogli · 2025-06-05T16:49:38Z

hathor/nanocontracts/storage/patricia_trie.py

+
+        last_match: bytes = b''
+
+        while True:


Should we add a maximum number of iterations to prevent an infinite loop caused by a bug?

Postponed thread.

msbrogli · 2025-06-05T16:52:36Z

hathor/nanocontracts/storage/contract_storage.py

+        self._trie: PatriciaTrie = trie
+
+        # Nano contract id
+        self.nc_id = nc_id


Rename to self.contract_id?

Postponed thread.

msbrogli · 2025-06-05T17:21:38Z

hathor/nanocontracts/storage/contract_storage.py

+    key: bytes
+
+    def __bytes__(self) -> bytes:
+        return _Tag.ATTR.value + hashlib.sha1(self.key).digest()


Should we keep using sha1? In fact, do we need to hash at all? I guess so because it is what guarantee that the tree will be balanced.

Postponed thread.

msbrogli · 2025-06-05T17:22:11Z

hathor/nanocontracts/storage/contract_storage.py

+    token_uid: bytes
+
+    def __bytes__(self) -> bytes:
+        return _Tag.BALANCE.value + self.token_uid


Should we assert that len(self.token_uid) in {1, 32}?

Postponed thread.

msbrogli · 2025-06-05T17:22:28Z

hathor/nanocontracts/storage/contract_storage.py

+    key: bytes
+
+    def __bytes__(self) -> bytes:
+        return _Tag.METADATA.value + hashlib.sha1(self.key).digest()


Same here. Should we keep using sha1? In fact, do we need to hash at all?

Postponed thread.

msbrogli · 2025-06-05T17:24:42Z

hathor/nanocontracts/storage/contract_storage.py

+    token_uid: bytes
+
+    def __bytes__(self) -> bytes:
+        return _Tag.BALANCE.value + self.token_uid


Should we hash the token_uid? Or is it random enough to guarantee that the tree will be balanced?

Postponed thread.

msbrogli · 2025-06-05T17:25:10Z

hathor/nanocontracts/storage/contract_storage.py

+
+
+@dataclass(frozen=True, slots=True)
+class MetadataKey(TrieKey):


Move this TrieKey closer to the others.

Postponed thread.

glevco · 2025-06-06T19:59:14Z

hathor/nanocontracts/storage/backends.py

+    def __len__(self) -> int:
+        it = self._db.iterkeys()
+        it.seek_to_first()
+        return sum(1 for _ in it)


Is this used anywhere? If not, it could be removed because it runs in O(n). If we do need it, we should implement a stats column that keeps a count.

Postponed thread.

I couldn't find any use of it.

hathor/nanocontracts/storage/block_storage.py

glevco · 2025-06-06T20:13:16Z

hathor/nanocontracts/storage/contract_storage.py

+class TrieKey(ABC):
+    @abstractmethod
+    def __bytes__(self) -> bytes:
+        raise NotImplementedError


The block storage doesn't use this and it uses NamedTuples intead of dataclasses. We should probably unify this.

Postponed thread.

glevco · 2025-06-06T20:15:41Z

hathor/nanocontracts/storage/contract_storage.py

+        assert isinstance(balance, MutableBalance)
+        return balance
+
+    def get_all_balances(self) -> dict[BalanceKey, Balance]:


This is using internal methods from the trie, review whether they should be public or if this method should be moved.

Postponed thread.

glevco · 2025-06-06T20:19:10Z

LGTM ✅

jansegre · 2025-06-06T19:36:58Z

hathor/nanocontracts/storage/maybedeleted_nc_type.py

We had talked about shaving off a byte from this class' serialization by removing the tag and using an "empty byte sequence" to represent a DeletedKey. It's possible to do that "outside" of the NCType (de)serialization system. It changes the root-id hash, but for now that is acceptable.

I think this isn't important just noting so we don't forget.

jansegre · 2025-06-06T20:30:03Z

hathor/nanocontracts/storage/changes_tracker.py

+        return self.melt is _NCAuthorityState.REVOKED
+
+
+class NCChangesTracker(NCContractStorage):


Or rather a NCContractStorageInterface (not Protocol), but yeah, I agree.

jansegre · 2025-06-06T20:45:08Z

hathor/nanocontracts/storage/changes_tracker.py

+    @override
+    def check_if_locked(self) -> None:
+        if self.has_been_commited:
+            raise RuntimeError('you cannot change any value after the commit has been executed')
+        elif self.has_been_blocked:
+            raise RuntimeError('you cannot change any value after the changes have been blocked')


I think we could use type-states for this, similar to the pattern used by Balance/MutableBalance, we could expose a NCContractStorageLocked which only has read methods (which it forwards to its nc_storage: NCContractStorageInterface) and a NCContractStorageUnlocked which also exposes methods with side effects, lock and unlock methods can convert one into the other (and even prevent double-lock/double-unlock).

jansegre · 2025-06-06T20:45:51Z

hathor/nanocontracts/storage/backends.py

+    def __len__(self) -> int:
+        it = self._db.iterkeys()
+        it.seek_to_first()
+        return sum(1 for _ in it)


I couldn't find any use of it.

jansegre · 2025-06-06T21:10:08Z

hathor/nanocontracts/storage/contract_storage.py

+    def get_root_id(self) -> bytes:
+        """Return the current merkle root id of the trie."""
+        return self._trie.root.id


I think we need a combinations of tests to cover the "stability" of this method. Even though breaking it currently doesn't break the network (in the sense that the blockchain can still sink and has the same observable state), it will break the storage and require some sort of migration. So we have to make it very clear when a PR can cause that.

jansegre · 2025-06-06T21:14:54Z

hathor/nanocontracts/storage/node_nc_type.py

+    # XXX: id is not optional, we're indicating that only nodes with id can be stored
+    _id: NCType[NodeId]


This is currently the only reasons for this class to be "manual", maybe some tweaks to Node could simplify this and get rid of this module.

glevco self-assigned this May 30, 2025

glevco requested review from jansegre and msbrogli as code owners May 30, 2025 17:22

glevco added this to Hathor Network May 30, 2025

github-project-automation bot moved this to Todo in Hathor Network May 30, 2025

glevco changed the title ~~feat(nano): implement storage module~~ feat(nano): implement storage module [part 4] May 30, 2025

glevco moved this from Todo to In Progress (Done) in Hathor Network May 30, 2025

glevco force-pushed the feat/nano/nc-types branch from 3691e76 to 8c6b9d4 Compare June 3, 2025 02:24

glevco force-pushed the feat/nano/storages branch from ccbd984 to 646105d Compare June 3, 2025 02:26

glevco force-pushed the feat/nano/nc-types branch from 8c6b9d4 to d29507c Compare June 3, 2025 18:41

glevco force-pushed the feat/nano/storages branch from 646105d to 5a6ca04 Compare June 3, 2025 18:43

Base automatically changed from feat/nano/nc-types to master June 3, 2025 19:53

glevco force-pushed the feat/nano/storages branch from 5a6ca04 to ded9471 Compare June 3, 2025 19:58

msbrogli requested changes Jun 5, 2025

View reviewed changes

github-project-automation bot moved this from In Progress (Done) to In Review (WIP) in Hathor Network Jun 5, 2025

msbrogli previously approved these changes Jun 5, 2025

View reviewed changes

glevco force-pushed the feat/nano/storages branch 2 times, most recently from cf70f19 to 72d6938 Compare June 6, 2025 19:13

feat(nano): implement storage module

d2eaf78

glevco force-pushed the feat/nano/storages branch from 72d6938 to d2eaf78 Compare June 6, 2025 19:54

glevco dismissed msbrogli’s stale review via f65c9a2 June 6, 2025 20:08

glevco commented Jun 6, 2025

View reviewed changes

fix(nano): address part 4 issues (#258)

24fa883

glevco force-pushed the feat/nano/storages branch from f65c9a2 to 24fa883 Compare June 6, 2025 20:18

msbrogli approved these changes Jun 6, 2025

View reviewed changes

jansegre approved these changes Jun 6, 2025

View reviewed changes

glevco moved this from In Review (WIP) to In Review (Done) in Hathor Network Jun 6, 2025

jansegre merged commit 5821d59 into master Jun 6, 2025
6 of 8 checks passed

jansegre deleted the feat/nano/storages branch June 6, 2025 22:15

github-project-automation bot moved this from In Review (Done) to Waiting to be deployed in Hathor Network Jun 6, 2025

glevco moved this from Waiting to be deployed to In Review (Done) in Hathor Network Jun 9, 2025

glevco moved this from In Review (Done) to Waiting to be deployed in Hathor Network Jun 9, 2025

glevco restored the feat/nano/storages branch June 9, 2025 12:08

glevco deleted the feat/nano/storages branch June 9, 2025 12:08

jansegre mentioned this pull request Jul 22, 2025

Release-candidate v0.64.0-rc.1 #1331

Merged

2 tasks

jansegre moved this from Waiting to be deployed to Done in Hathor Network Jul 22, 2025

jansegre mentioned this pull request Aug 7, 2025

Release v0.64.0 #1352

Merged

2 tasks

		raise NotImplementedError


		class MemoryNodeTrieStore(NodeTrieStore):



		@dataclass(frozen=True, slots=True)
		class MetadataKey(TrieKey):

		return self.melt is _NCAuthorityState.REVOKED


		class NCChangesTracker(NCContractStorage):

		# XXX: id is not optional, we're indicating that only nodes with id can be stored
		_id: NCType[NodeId]

Conversation

glevco commented May 30, 2025

Motivation

Acceptance Criteria

Checklist

Uh oh!

codecov bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glevco commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

codecov bot commented Jun 3, 2025 •

edited

Loading