The `chia.consensus` module no longer depends upon `chia.full_node` #19671

richardkiss · 2025-06-03T20:12:01Z

Prior to this change, there were several dependencies of chia.full_node in the chia.consensus module.

coveralls-official · 2025-06-03T21:32:49Z

Pull Request Test Coverage Report for Build 15426877319

Details

224 of 261 (85.82%) changed or added relevant lines in 31 files are covered.
17 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.009%) to 91.229%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
chia/consensus/block_height_map_abc.py	21	28	75.0%
chia/consensus/coin_store_abc.py	31	41	75.61%
chia/consensus/block_store_abc.py	52	72	72.22%

Files with Coverage Reduction	New Missed Lines	%
chia/daemon/client.py	1	74.72%
chia/server/node_discovery.py	1	81.93%
chia/full_node/full_node.py	2	88.23%
chia/util/files.py	2	91.38%
chia/rpc/rpc_server.py	3	88.54%
chia/daemon/server.py	8	83.46%

Totals
Change from base Build 15426193337:	0.009%
Covered Lines:	101506
Relevant Lines:	111148

💛 - Coveralls

arvidn · 2025-06-04T07:14:02Z

is there a vision of what chia.consensus should contain? because "consensus rules" are spread out virtually everywhere today

arvidn

I think it's reasonable to move and rename mempool_check_time_locks().
I don't see the rationale for moving BlockHeightMap out of Blockchain, it seems like you just make it more complicated to construct the Blockchain object, and I don't see an upside. We don't need the BlockHeightMap outside Blockchain, do we?
Also, I'm under the impression that we prefer Protocols over inheritance (I personally do at least). We have some precedence of using protocols as well.
However, at a higher level, why do you need an interface/protocol for these types? The main reason I can think if is if you want to be able to stub them out in tests, but I don't see that. It seems like added complexity with no upside, but perhaps I'm missing it.

arvidn · 2025-06-04T07:23:15Z

chia/full_node/block_height_map.py

it seems a lot simpler to just move this class into chia.consensus and use concrete types. We only have one class implementing the interface, and it doesn't look like we have any intention of adding more implementations, so we should just use the concrete type and keep things simple, I think

The reasoning is I want to keep storage details away from consensus. I actually think chia.consensus does a pretty good job of isolating the consensus stuff (not including mempool, which IMO isn't actually consensus. I prefer to narrowly define consensus as "blockchain validation"). This opens the door to (for example) creating a completely RAM-based consensus mechanism that just stores all the data in python dictionaries.

We could move the three storage classes into consensus. If you look at the implementations of BlockHeightMap, CoinStore and BlockStore, they are, IMO, ugly and messy and bogged-down in SQLite3 administrivia that distracts from the understanding of how consensus works. I'd actually quite like to move DBWrapper and friends etc. into a new module called chia.sqlite3 or chia.db, to isolate the storage implementations into a black box.

Note that I am working on a new implementation of CoinStore right now (the rocks_db thing) and having an ABC will make it easier to split CoinStore into ConsensusCoinStore (or whatever) and ExplorerStore (indices of parent coin info and puzzle hashes) which I'm sure you've heard me harp on about several times now.

I think it's a bit funny that python supports ABC and Protocol. It makes it hard to know which one to use. ChatGPT recommended ABC for this case. I guess Protocol is useful for built-in types like saying "I need a thing that looks like a bytes object in the following ways...". For CoinStore and friends, I kind of prefer ABC because it allows default implementations (for example, get_coin_record could easily have a default imp that uses get_coin_records with very little loss in efficiency), and it makes it easier to determine exactly what you have to write and what is still missing without resorting to mypy (which for me always produces a lot of noise that signal gets lost in because I'm probably doing something wrong).

An argument for avoiding SQLite3 code in consensus: it's not side-effect free. It's much easier to reason about pure functional code that has no side effects. The ABC is side-effect free (well, except I guess it mutates the state of the object, which I guess is generally seen as okay). Any disk-based stuff is loaded with side effects.

Additionally, this change will make it easier to provide a transition path where we temporarily support the SQLite3 DB and the RocksDB for CoinStore (and if we move more stuff to rocksdb in future, it helps there too).

ABC is ancient. Protocol is more recent. i encourage protocols since they don't use inheritance and whatever related to that.

richardkiss · 2025-06-04T18:39:13Z

chia/consensus/coin_store_abc.py

+    This is a substitute for importing from chia.full_node.coin_store directly.
+    """
+
+    db_wrapper: DBWrapper2


This line should be removed for sure.

richardkiss · 2025-06-04T18:40:04Z

chia/consensus/coin_store_abc.py

+    This is a substitute for importing from chia.full_node.coin_store directly.
+    """
+
+    db_wrapper: DBWrapper2


Suggested change

db_wrapper: DBWrapper2

richardkiss · 2025-06-05T21:02:02Z

chia/full_node/block_store.py

+        """Provide a transaction context."""
+
+        async with self.db_wrapper.writer() as transaction:
+            yield transaction


Please check this part carefully. I think it's right, but I got a weird transient error on one test set: py3.10 Mac OS Intel simulator, something about trying to operate on a closed DB. I don't recall having seen that before so it's possible it's related to this.

richardkiss · 2025-06-05T21:02:28Z

chia/full_node/block_store.py

-                for row in await cursor.fetchall():
-                    block_rec = BlockRecord.from_bytes(row[1])
-                    all_blocks[block_rec.header_hash] = block_rec
+        batch_size = self.db_wrapper.host_parameter_limit


Batching moved down into here.

github-actions · 2025-10-07T11:06:12Z

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

richardkiss requested a review from arvidn June 3, 2025 20:12

richardkiss requested a review from a team as a code owner June 3, 2025 20:12

richardkiss added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Jun 3, 2025

richardkiss requested a review from altendky June 3, 2025 20:12

aqk previously approved these changes Jun 3, 2025

View reviewed changes

arvidn reviewed Jun 4, 2025

View reviewed changes

richardkiss commented Jun 4, 2025

View reviewed changes

richardkiss dismissed aqk’s stale review via 6625f84 June 5, 2025 05:30

richardkiss force-pushed the tach branch 15 times, most recently from f842243 to 914a487 Compare June 5, 2025 20:07

richardkiss commented Jun 5, 2025

View reviewed changes

richardkiss force-pushed the tach branch 7 times, most recently from b631ce3 to 1244bd0 Compare July 30, 2025 20:50

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 1, 2025

richardkiss force-pushed the tach branch from 1244bd0 to f5709f8 Compare August 20, 2025 21:49

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 20, 2025

richardkiss added 2 commits August 20, 2025 14:57

harvester_api_protocol

a9ec206

move to apis

9433e6c

richardkiss force-pushed the tach branch from f5709f8 to 9433e6c Compare August 20, 2025 22:03

richardkiss added 5 commits August 20, 2025 15:14

Remove chia.apis => chia.harvester dependency.

0dc30a6

TimelordApiStub

70fbd21

farmer

d91b889

full node stubs

bb77c53

introducer

e90bfbf

richardkiss force-pushed the tach branch from e6ae504 to e90bfbf Compare August 20, 2025 23:11

wallet stubs

32acdc2

richardkiss force-pushed the tach branch from 3bed260 to 5909b05 Compare August 20, 2025 23:24

NotImplemented

1921bc9

richardkiss force-pushed the tach branch 3 times, most recently from f877695 to 0c3e3a2 Compare August 21, 2025 06:18

tests

5a95eca

richardkiss force-pushed the tach branch from 0c3e3a2 to 5a95eca Compare August 21, 2025 18:25

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 22, 2025

github-actions bot added the stale-pr Flagged as stale and in need of manual review label Oct 7, 2025

The chia.consensus module no longer depends upon chia.full_node #19671

Are you sure you want to change the base?

The chia.consensus module no longer depends upon chia.full_node #19671

Uh oh!

Conversation

richardkiss commented Jun 3, 2025

Uh oh!

coveralls-official bot commented Jun 3, 2025

Pull Request Test Coverage Report for Build 15426877319

Details

💛 - Coveralls

Uh oh!

arvidn commented Jun 4, 2025

Uh oh!

arvidn left a comment

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!

github-actions bot commented Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

The `chia.consensus` module no longer depends upon `chia.full_node` #19671

The `chia.consensus` module no longer depends upon `chia.full_node` #19671