Make access to Nakamoto staging blocks restricted to one single pub fn

[jferrant]

As part of AAC, determined the desired code path to test should be accept_block (which is called in the p2p_broadcast function for miners, and in process_new_nakamoto_block_ext in the Relayer) and that these seemed to be the only points of entry to the staging blocks db. This PR just makes it clear that this is actually the case by making all other writing function calls private in nakamoto block staging db.

Confirmed that calling store_block_if_better on a shadow block has the same effect as just doing store_block. Therefore, we can use it as the single point of entry. I also updated the NakamotoChainState::accept_block fn sig to be a bit more clear (at least to me), but I can revert that if the old fn sig is preferred.

[jcnelson]

LGTM, and looks a lot safer too. Thanks!

[fdefelici]

I really like this PR. It clarifies a lot of aspects around the block storage logic and introduces a clear single entry point for it. Great work on that part!

That said, I have some concerns about moving more business logic into the database layer.

In my view, the DB layer should primarily expose CRUD operations and enforce data integrity (for example, the methods made private in this PR fit well within that responsibility). However, with this change, the DB layer seems to have gained some additional business responsibilities.

This also has implications:

• Maybe less clarity on which layer we should write business logic.
• NakamotoChainState tests now depend (explicitly) on the DB layer’s business logic to run correctly.
• Testing the DB itself becomes a bit more trickier (considering our test organization), since some CRUD methods are now private, we can only test DB behavior indirectly through its business methods (or integration tests). Of course, we could still work around this by adding tests in the same module or child module, requiring different test organization.

To be clear, I’m not suggesting we block this PR. I think the direction is overall positive. I just wanted to open a discussion on this point, since we have similar situations in other DB implementations, and it might be worth aligning on a common approach.

[jferrant]

I really like this PR. It clarifies a lot of aspects around the block storage logic and introduces a clear single entry point for it. Great work on that part!

That said, I have some concerns about moving more business logic into the database layer.

I had the same thought while writing this PR. I hink its better to make the DB operations independent of chainstate logic, but was having a hard time enforcing the privacy restrictions i wanted as a result/I wanted to quickly prove that accept_block was truly the only point of insertion into the db. I do think its worth revisiting/additional effort to enforce better separation while keeping tighter restrictions on the db access. Let me see what I can do today cause I prob didn't give it enough thought.

[jcnelson]

That said, I have some concerns about moving more business logic into the database layer.

Can you elaborate more on this point? What business logic features do you believe are present in the database layer that could be moved out?

[jferrant]

That said, I have some concerns about moving more business logic into the database layer.

Can you elaborate more on this point? What business logic features do you believe are present in the database layer that could be moved out?

I was thinking specifically store_block_if_better (which mostly calls already existing db functions though). However it does have logic around orphaned blocks, signing weight, etc. that seem a bit like they shouldn't be in the db transaction? But not sure how else to set this up. I would expect the outer caller to do these sorts of checks but then it means exposing all those inner db calls...so this is why I ended up where I ended up

[jcnelson]

I was thinking specifically store_block_if_better (which mostly calls already existing db functions though). However it does have logic around orphaned blocks, signing weight, etc. that seem a bit like they shouldn't be in the db transaction? But not sure how else to set this up. I would expect the outer caller to do these sorts of checks but then it means exposing all those inner db calls...so this is why I ended up where I ended up

Right -- store_block_if_better is part of the transaction logic precisely because it has to happen within a database transaction. It's performing reads and writes on multiple records which must happen atomically with respect to other reads and writes. This atomicity requirement is what drives the current factoring. Otherwise, as you pointed out, we'd end up polluting the business logic with a lot of nitty-gritty details of transaction management, which I think is a worse outcome than the status quo since it creates more opportunities for accidental consensus bugs. To speak to @fdefelici's point, I think of this function as one of many different "update" methods exposed by the DB.

[fdefelici]

Right -- store_block_if_better is part of the transaction logic precisely because it has to happen within a database transaction. It's performing reads and writes on multiple records which must happen atomically with respect to other reads and writes. This atomicity requirement is what drives the current factoring. Otherwise, as you pointed out, we'd end up polluting the business logic with a lot of nitty-gritty details of transaction management, which I think is a worse outcome than the status quo since it creates more opportunities for accidental consensus bugs. To speak to @fdefelici's point, I think of this function as one of many different "update" methods exposed by the DB.

I think the current factoring was primarily driven by the goal of centralizing this piece of business logic, which is definitely a good direction.
That said, I don’t see any transaction management details here (just a series of CRUD operations on the DB). The actual transaction handling (creation, commit, rollback) still happens outside (for instance, in NakamotoChainState).

My main concern is simply where we choose to place this centralized logic.

In my view, deciding whether one block is “better” than another shouldn’t be the DB’s responsibility, because that logic could evolve in future epochs. This effectively gives the DB two distinct “reasons to change” (one business-related, one data-related) which can blur its purpose.

As @jferrant mentioned, it might be that for now the only practical place to host this logic (due to code constraints) is within the transaction layer. I assume because of the dependency with NakamotoStagingBlocksTx::add_shadow_block operation.

More broadly, beyond this specific function, what concerns me most is that it’s not fully clear (at least to me) where this kind of logic is expected to live going forward. For example:

• NakamotoChainState and StacksChainstate both contain business logic around DB/TXS operations.
• NakamotoStagingBlocksTx also embeds some business logic on top of the staging DB.
• NakamotoStagingBlocksConnRef focuses more purely on DB logic.
• Some DB operations that feel “natural” to expose at the DB layer have been made private.
• These changes are already having an effect on test organization (as described in my earlier comment).

To illustrate what I mean about boundary clarity:

• In NakamotoChainState::accept_block, there’s logic that manipulates the staging DB directly. Following the same principle as in this PR (or the one you shared above), that logic could arguably move into NakamotoStagingBlocksTx instead.
• Comparing NakamotoStagingBlocksTx and NakamotoStagingBlocksConnRef, it’s not immediately clear where DB logic (read query) belong. Trying to extract a "rule of thumb" it seems that NakamotoStagingBlocksTx should be used for write operations, and NakamotoStagingBlocksConnRef for read operations, but for instance, yet we have cases like NakamotoStagingBlocksTx::has_nakamoto_block_with_block_hash (a read operation).

Overall, these boundaries feel a bit opaque, which can make the code harder to reason about and maintain, at least from my current understanding of the system.

That said, this PR is still a solid improvement and a positive step forward.
As mentioned in my initial comment, this discussion wasn’t meant to block it, so I’ll go ahead and approve.

Happy to continue this conversation if others have additional thoughts.

[codecov]

Codecov Report

❌ Patch coverage is 87.96296% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.09%. Comparing base (c5cca10) to head (61c819a).
⚠️ Report is 8 commits behind head on develop.

Files with missing lines	Patch %	Lines
...tackslib/src/chainstate/nakamoto/staging_blocks.rs	75.55%	11 Missing ⚠️
stackslib/src/net/relay.rs	50.00%	2 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6585      +/-   ##
===========================================
+ Coverage    76.19%   77.09%   +0.89%     
===========================================
  Files          571      571              
  Lines       351593   351577      -16     
===========================================
+ Hits        267882   271032    +3150     
+ Misses       83711    80545    -3166     

Files with missing lines	Coverage Δ
stacks-node/src/nakamoto_node/miner.rs	69.19% <100.00%> (-2.83%)	⬇️
stackslib/src/chainstate/nakamoto/mod.rs	83.21% <100.00%> (+0.21%)	⬆️
stackslib/src/chainstate/nakamoto/shadow.rs	61.96% <100.00%> (ø)
stackslib/src/chainstate/nakamoto/tests/mod.rs	99.13% <100.00%> (ø)
stackslib/src/chainstate/nakamoto/tests/node.rs	83.83% <100.00%> (-0.06%)	⬇️
stackslib/src/net/relay.rs	67.71% <50.00%> (+2.24%)	⬆️
...tackslib/src/chainstate/nakamoto/staging_blocks.rs	82.83% <75.55%> (-0.81%)	⬇️

... and 98 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5cca10...61c819a. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.