feat: clarity block connection depend on backing store interface

[bestmike007]

Description

Previously ClarityBlockConnection and ClarityReadonlyConnection are depending on concrete implementation of the ClarityBackingStore trait.

In this PR, a ClarityBackingStoreTransaction trait is introduced, and ClarityBlockConnection and ClarityReadonlyConnection are changed to depend on interfaces rather than implementations.

This will be very helpful to extended use cases to reuse block consensus logic, e.g. implementing an ephemeral WritableMarfStore for simulation.

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[jcnelson]

This will be very helpful to extended use cases to reuse block consensus logic, e.g. implementing an ephemeral WritableMarfStore for simulation.

PR #6365 already has an ephemeral WritableMarfStore implementation.

[bestmike007]

This is not an ephemeral WritableMarfStore implementation; it removes the dependency on concrete implementations.

[bestmike007]

This is what I have in mind to support using ephemeral storage (I copied your implementation of EphemeralMarfStore):
bestmike007/stacks-core@feat/clarity-block-connection-depend-on-backing-store-interface...bestmike007:stacks-core:feat/ephemeral-marf-store

By removing the dependency to specific implementation, it's easier to add and use different implementation.

[jcnelson]

This is not an ephemeral WritableMarfStore implementation; it removes the dependency on concrete implementations.

Yes, I understand that.

Forgive me if this is asking you to repeat something you may have explained to someone else, but can you provide us with some context on why you want to add a pub trait, instead of going with the enum WritableMarfStore in #6365? For example, do you have downstream code on your end (i.e. that uses stackslib as a dependency) that would benefit from supplying the Clarity VM with a trait implementation? The reason I'm asking is because if we accept this PR, we'd need to support and document the pub trait as a public API for downstream consumers of stackslib for the foreseeable future. To do a good job at this, we'd want to spend some time making sure the trait definition is sufficiently well-adapted to known external use-cases (I'm told Zest also wants something like this?).

[bestmike007]

You mean ClarityBackingStoreTransaction right? Using trait over enum means that there can be infinite possibilities of implementations. Among which the EphemeralMarfStore in #6365 is one of them.

In STXER, we do have another overlay implementation for ClarityBackingStore which store the data for a fork chain in another persisted sqlite database. And I also had discussion with @aldur maybe we can have another less secure follower node which store data in postgres, redis, and s3, so it would be easier for data developers/scientists to replicate the data to different storage systems for frontend integration/analytics/dashboard purposes.

Another reason I prefer trait over enum WritableMarfStore is that, I think it's better if we follow the open-closed principle. We should avoid modifying fully tested modules when possible, to prevent introducing unexpected bugs.

[jcnelson]

Hi @bestmike007,

Using trait over enum means that there can be infinite possibilities of implementations

No one is questioning this in principle. What I'm questioning is whether or not the proposed trait in this PR represents the right abstraction for addressing the problem(s) at hand. It's a question we need to answer before merging, because a pub trait is potentially something that will need to be maintained for a long time, depending on downstream usage (whereas a pub enum is not).

In STXER, we do have another overlay implementation for ClarityBackingStore which store the data for a fork chain in another persisted sqlite database. And I also had discussion with @aldur maybe we can have another less secure follower node which store data in postgres, redis, and s3, so it would be easier for data developers/scientists to replicate the data to different storage systems for frontend integration/analytics/dashboard purposes.

Thank you for this; this is useful context. Now with this application and other potential applications in mind, let's look at the trait definition itself:

pub trait ClarityBackingStoreTransaction: ClarityBackingStore {
    fn rollback_block(self: Box<Self>);
    fn rollback_unconfirmed(self: Box<Self>) -> Result<()>;
    fn commit_to(self: Box<Self>, final_bhh: &StacksBlockId) -> Result<()>;
    fn commit_unconfirmed(self: Box<Self>);
    fn commit_mined_block(self: Box<Self>, will_move_to: &StacksBlockId) -> Result<()>;
    fn seal(&mut self) -> TrieHash;
}

Given this context, I'm not convinced that this represents a useful level of abstraction to represent writes against a Clarity backing store. In general, this abstraction is tightly coupled to implementation decisions in the Stacks node that aren't relevant to these possible applications. Specifically:

• seal() exists as an implementation optimization today -- it lets the node determine when the trie root hash is calculated, thereby "sealing" the trie so it cannot be modified afterwards. Why would another trait implementation need to implement seal()?

• rollback_unconfirmed() and commit_unconfirmed() specifically pertain to handling unconfirmed microblock streams in Stacks 2.x. The abstraction here is coding to a deprecated implementation.

• commit_mined_block() specifically pertains to a code path used by the miner in order to ensure that block tries are stored in such a way that they do not prevent the same block from being added to the chainstate (since mined blocks are not guaranteed to be accepted by the signers). This is implementation-specific, and likely wouldn't be useful to the aforementioned applications (since they don't mine blocks).

That all said, I'm still very interested in helping make it easier for STXER to use its own Clarity storage implementation. We just need to come up with the right abstraction that won't couple STXER to Stacks implementation details :)

[bestmike007]

I'm not convinced that this represents a useful level of abstraction to represent writes against a Clarity backing store. In general, this abstraction is tightly coupled to implementation decisions in the Stacks node that aren't relevant to these possible applications.

I could be wrong but I'm thinking differently. This trait abstract the abilities that stacks implementation required in order to process blocks and transactions. So it is tightly coupled to stacks implementation decisions, possible applications are not supposed to use it directly, they implement the trait so that they can reuse the logic in stackslib.

However, I agree with you that it might not be a useful level of abstraction, functions defined in the trait cover different code paths in the stacks implementation. A specific application might not need to implement all of them based on the usage, for example if I'm not running a miner then I probably don't need to implement commit_mined_block(). Similarly when STXER implement the existing ClarityBackingStore trait, it does not implement *_with_proof functions. So probably adding comments to the trait functions and let extension application developers decide whether to implement or not, is the best thing we can do for now. We can mark the trait experimental and we can definitely improve it later, for example moving some of the implementations to ClarityBlockConnection if possible.

[moodmosaic]

And I also had discussion with aldur maybe we can have another less secure follower node which store data in postgres, redis, and s3, so it would be easier for data developers/scientists to replicate the data to different […]

@bestmike007, is there a link to the comment(s)/discussion?

[moodmosaic]

A specific application might not need to implement all of them based on the usage […] adding comments to the trait functions and let extension application developers decide […]

A (more robust) alternative is having many smaller traits, each describing one role vs having fewer/bigger header interfaces which are hard to evolve (add/removing, or changing methods tends to break downstream code).

[bestmike007]

@moodmosaic we talked on Signal, so no links. @aldur was asking if I had more ideas to improve stacks and the ecosystem, and here's what I responded:

So besides what we have in STXER, I also have another idea to develop a separate follower node, which can use less secure crates like tokio, axum, database thread pooling, etc. It simply retrieve blocks from the core and replay them and choose different storage, e.g. postgres, s3, redis. This could help web3 developers a lot.
If stacks-core can be better refactored, it'll help a lot. Many places in the stacks core are now depending on implementations instead of the interface, this makes extension really difficult.
And there are a lot of limits for Clarity language, web assembly will help a lot and I assume you'll raise the block budget. Heavy computations will be easier, but I think you still need to consider supporting offchain computing, like TEE which helps oracles a lot, and probably zkSnark as well. Basically it's adding more crypto related Clarity functions.

[bestmike007]

having many smaller traits

I thought about this as well, but ClarityBlockTransaction is using all of them. Not easy to split them before we take a deeper look at current implementation and see if we can decouple more.

[bestmike007]

I tend to keep PRs small, so consider this a small step toward the bigger goal.

[kantai]

The reason I'm asking is because if we accept this PR, we'd need to support and document the pub trait as a public API for downstream consumers of stackslib for the foreseeable future.

I think I disagree with this -- we have many traits throughout the codebase, and we change them without too much thought as to how it might impact downstream dependencies. While I agree that it may be disruptive to people who depend on those traits, it doesn't seem to have generated much push back when we've done so previously. We could think about changing policies regarding trait changes, but I don't think we need to do that now, nor do I think that we need to evaluate this PR in the context of such a change. Basically: there are already traits in the codebase, we already change them if they don't fit their purpose (or the interface changes), and so I don't think we should apply that standard to this PR.

However, I agree with you that it might not be a useful level of abstraction, functions defined in the trait cover different code paths in the stacks implementation. A specific application might not need to implement all of them based on the usage, for example if I'm not running a miner then I probably don't need to implement commit_mined_block(). Similarly when STXER implement the existing ClarityBackingStore trait, it does not implement *_with_proof functions. So probably adding comments to the trait functions and let extension application developers decide whether to implement or not, is the best thing we can do for now.

Yes, I think adding comments to the trait functions would help in these cases, but overall I agree with you about the level of abstraction in this trait: the trait is meant to capture the interface between the VM and the block processing, so of course it will end up pretty closely tied to the implementation we have today.

Overall, I'm perfectly happy with the approach this PR has taken so far. If we want to iterate on the trait definition, I think we can do that in follow up PRs and should do so without too much worry about downstream implementors. If it gets to be way too much thrashing, people can complain and then we can come up with a better policy for handling trait changes, but until that happens, I'd rather we have this trait than work through implementations that use enums.

[jcnelson]

I think I disagree with this -- we have many traits throughout the codebase, and we change them without too much thought as to how it might impact downstream dependencies.

That's because the principal consumers of these traits have tended to be other modules within stacks-core. I'm not aware of any substantial development efforts to factor stackslib for consumption with other node implementations, beyond what @bestmike007 discussed with @aldur over Signal (which would mean that this conversation is only just now surfacing). I think it's worthwhile to consider the magnitude of the consequences of changing a public trait when its principal consumers are internal modules, versus external node implementations.

We could think about changing policies regarding trait changes, but I don't think we need to do that now, nor do I think that we need to evaluate this PR in the context of such a change.

The difference between then and now is that this PR would treat this trait as a public API within stackslib, in a way that existing traits (despite being public) are not treated.

That said, I am generally supportive of this goal. stackslib can and should be factored into multiple crates with well-defined abstractions and interfaces between them. It would certainly make the quality of life better -- for example, it would be easier to fuzz each crate separately and would speed up compilation times.

I'm just asking that if we are to go down this path, we think carefully about what these well-defined abstractions and interfaces will be, since it won't be just stacks-core contributors who will have to live with them.

@bestmike007 I can spend some time working with you on this.

[bestmike007]

Thanks @jcnelson!

stackslib can and should be factored into multiple crates with well-defined abstractions and interfaces between them

I completely agree — this PR is just a small step in that direction.

The difference between then and now is that this PR would treat this trait as a public API within stackslib, in a way that existing traits (despite being public) are not treated.

That’s a fair point. I’d note that STXER has already implemented the existing ClarityBackingStore trait, and even when it changed once, it didn’t cause issues for STXER.

That said, I don’t think this trait should be considered a finalized “public API” yet. I agree we should be deliberate about the abstractions. Do you already have a better trait definition (or direction) in mind?

[jcnelson]

Just an update here -- I'm working on a series of traits as part of #6365 that should (hopefully) make it a lot easier to plug in external MARF and ClarityBackingStore implementations. I'll post here when they're ready for review.

[jcnelson]

Okay, so here's what I have in #6365:

/// A MARF store transaction for a chainstate block's trie.
/// This transaction instantiates a trie which builds atop an already-written trie in the
/// chainstate.  Once committed, it will persist -- it may be built upon, and a subsequent attempt
/// to build the same trie will fail.
///
/// The Stacks node commits tries for one of three purposes:
/// * It processed a block, and needs to persist its trie in the chainstate proper.
/// * It mined a block, and needs to persist its trie outside of the chainstate proper. The miner
/// may build on it later.
/// * It processed an unconfirmed microblock (Stacks 2.x only), and needs to persist the
/// unconfirmed chainstate outside of the chainstate proper so that the microblock miner can
/// continue to build on it and the network can service RPC requests on its state.
///
/// These needs are each captured in distinct methods for committing this transaction.
pub trait ClarityMarfStoreTransaction {
    /// Commit all inserted metadata and associate it with the block trie identified by `target`.
    /// It can later be deleted via `drop_metadata_for()` if given the same taret.
    /// Returns Ok(()) on success
    /// Returns Err(..) on error
    fn commit_metadata_for_trie(&mut self, target: &StacksBlockId) -> InterpreterResult<()>;

    /// Drop metadata for a particular block trie that was stored previously via `commit_metadata_to()`.
    /// This function is idempotent.
    ///
    /// Returns Ok(()) if the metadata for the trie identified by `target` was dropped.
    /// It will be possible to insert it again afterwards.
    /// Returns Err(..) if the metadata was not successfully dropped.
    fn drop_metadata_for_trie(&mut self, target: &StacksBlockId) -> InterpreterResult<()>;

    /// Compute the ID of the trie being built.
    /// In Stacks, this will only be called once all key/value pairs are inserted (and will only be
    /// called at most once in this transaction's lifetime).
    fn seal_trie(&mut self) -> TrieHash;

    /// Drop the block trie that this transaction was creating.
    /// Destroys the transaction.
    fn drop_current_trie(self);

    /// Drop the unconfirmed state trie that this transaction was creating.
    /// Destroys the transaction.
    ///
    /// Returns Ok(()) on successful deletion of the data
    /// Returns Err(..) if the deletion failed (this usually isn't recoverable, but recovery is up
    /// to the caller)
    fn drop_unconfirmed(self) -> InterpreterResult<()>;

    /// Store the processed block's trie that this transaction was creating.
    /// The trie's ID must be `target`, so that subsequent tries can be built on it (and so that
    /// subsequent queries can read from it).  `target` may not be known until it is time to write
    /// the trie out, which is why it is provided here.
    ///
    /// Returns Ok(()) if the block trie was successfully persisted.
    /// Returns Err(..) if there was an error in trying to persist this block trie.
    fn commit_to_processed_block(self, target: &StacksBlockId) -> InterpreterResult<()>;

    /// Store a mined block's trie that this transaction was creating.
    /// This function is distinct from `commit_to_processed_block()` in that the stored block will
    /// not be added to the chainstate. However, it must be persisted so that the node can later
    /// build on it.
    ///
    /// Returns Ok(()) if the block trie was successfully persisted.
    /// Returns Err(..) if there was an error trying to persist this MARF trie.
    fn commit_to_mined_block(self, target: &StacksBlockId) -> InterpreterResult<()>;

    /// Persist the unconfirmed state trie so that other parts of the Stacks node can read from it
    /// (such as to handle pending transactions or process RPC requests on it).
    fn commit_unconfirmed(self);

    /// Commit to the current chain tip.
    /// Used only for testing.
    #[cfg(test)]
    fn test_commit(self);
}

/// Unified API common to all MARF stores
pub trait ClarityMarfStore: ClarityBackingStore {
    /// Instantiate a `ClarityDatabase` out of this MARF store.
    /// Takes a `HeadersDB` and `BurnStateDB` implementation which are both used by
    /// `ClarityDatabase` to access Stacks's chainstate and sortition chainstate, respectively.
    fn as_clarity_db<'b>(
        &'b mut self,
        headers_db: &'b dyn HeadersDB,
        burn_state_db: &'b dyn BurnStateDB,
    ) -> ClarityDatabase<'b>
    where
        Self: Sized,
    {
        ClarityDatabase::new(self, headers_db, burn_state_db)
    }

    /// Instantiate an `AnalysisDatabase` out of this MARF store.
    fn as_analysis_db(&mut self) -> AnalysisDatabase
    where
        Self: Sized,
    {
        AnalysisDatabase::new(self)
    }
}

/// A MARF store which can be written to is both a ClarityMarfStore and a
/// ClarityMarfStoreTransaction (and thus also a ClarityBackingStore).
pub trait WritableMarfStore:
    ClarityMarfStore + ClarityMarfStoreTransaction + BoxedClarityMarfStoreTransaction
{
}

To add a new writable MARF store (called ExampleMarfStore), you'd do this:

impl ClarityMarfStore for ExampleMarfStore { }
impl ClarityMarfStoreTransaction for ExampleMarfStore {
   /* implementations of the above functions */
}
impl WritableMarfStore for ExampleMarfStore { }

[bestmike007]

This is what I have in mind to support using ephemeral storage (I copied your implementation of EphemeralMarfStore): bestmike007/stacks-core@feat/clarity-block-connection-depend-on-backing-store-interface...bestmike007:stacks-core:feat/ephemeral-marf-store

By removing the dependency to specific implementation, it's easier to add and use different implementation.

How about this?