Factoring out a stateless, side-effect free validation library #35904

Problem

Today, consensus validation is tightly coupled to the node implementation. As a result, unit tests require substantial test setup, and validation cannot be reused independently of the node's state-management implementation.

This issue proposes refactoring the validation code into a stateless layer with explicit inputs while preserving most of the existing code, behavior, and architecture.

Testing

The resulting validation interface will provide a dedicated unit-testing surface. Consensus validation can be tested by supplying only the required consensus inputs through lightweight implementations of ChainView and CoinIndex, without constructing ChainstateManager, CBlockIndex, or CCoinsViewCache.

Refactoring plan

The following actions have been identified to be necessary towards the goal of providing a standalone validation library. For a large part, those actions are independent of each other and can be done independently, in any order.

1. Expose the validation interface

   Make the existing CheckBlockHeader, ContextualCheckBlockHeader, and ContextualCheckBlock functions part of the public validation interface by declaring them in validation.h.

2. Interface Segregation: Remove dependency on ChainstateManager

   ContextualCheck* functions need a ChainstateManager only to access the consensus parameters. Pass Consensus::Params instead and access them at the call site.

3. Make environmental dependencies explicit

   Pass the validation time explicitly to ContextualCheckBlockHeader instead of reading the system clock internally.

4. Replace CBlockIndex with ChainView

   Introduce a ChainView abstraction representing the chain history required by contextual validation. Wrap the existing CBlockIndex-based implementation to provide this interface.

5. Introduce CoinIndex

   Introduce a read-only CoinIndex abstraction for the UTXO lookups performed during validation. Wrap CCoinsViewCache to provide this interface.

6. Extract validation from ConnectBlock

   Extract the consensus-validation logic from ConnectBlock into a stateless operation depending only on the block and its explicit consensus inputs (Consensus::Params, ChainView, CoinIndex). Further abstractions may become necessary (async execution, caching).

   ConnectBlock remains responsible for orchestrating validation and applying the resulting state transition.

More Interface Segregation

Many consensus relevant types, like CBlock, CTransaction, or uint256 provide functionality that is not required for validation, like mutability, stringification, or construction from a string representation. Decoupling such functionality from the type and instead providing it as utilities for testing or the wallet will further narrow the surface of a validation API.

1. Rewrite deserialization code in a way that it does not mutate individual data members

   Current serdes code is hard to reason about because it heavily relies on overload resolution. It is also one of the very few places that impose a mutability requirement on many types. Here is a proof of concept that an alternative is possible: https://github.com/purpleKarrot/std-bitcoin/blob/master/src/serdes_decode.cpp

2. Rewrite test code that mutates individual data members

   Many tests span multiple test cases sharing the same testing data, where each test case performs an assertion after applying a modification to the shared test data. This is problematic because it is hard to diagnose test failures. This is probably the second place out of two where mutability is required. Rewriting those test cases to be independent also removes the mutability requirement.

3. Make primitive types like CBlock, CTransaction, COutPoint, etc immutable

   Types that no longer have the requirement that their data members need to be individually mutable, should no longer provide ways to modify them. Their data members should be made private, with public observers. CMutableTransaction should no longer be necessary by now.

4. A block is not a block header, and its transactions are not optional

   CBlock should have rather than be a CBlockHeader (inheritance establishes an is-a relationship). CBlock should further own CTransaction objects, rather than pointers which could be nullptr. CBlock and CTransaction may be implemented in terms of std::shared_ptr<const impl> internally and provide a never-empty guarantee as part of their invariant.

5. Replace ToString functions with std::formatter specializations

6. Factor out string parsing helpers

   The use case for constructing a type from a string representation is very narrow. Performing the latter in a constant expression is exclusively required for testing. Such functionality should live in utilities rather than the constructor. User-defined literals are also good candidates for such utilities.

Relation to the Bitcoin Kernel library

The validation library provides stateless consensus validation. The Bitcoin Kernel library can build on it by providing concrete implementations of ChainView and CoinIndex while retaining responsibility for state management.

The problem you seem to be identifying here is, to my understanding:

• validating unit tests require substantial setup
• validation code cannot be used independently of chainstate

Those seem to be somewhat plausible but fairly minor advantages to me: the impact is that tests are slower to run (and that we therefore might have fewer tests run less often), and that third-party code is more complicated.

To solve that problem, you then spend far a lot of time describing a large refactoring of consensus code to solve that problem. That is a very high risk approach, and when it gains only fairly small advantages, I think very much the wrong approach.

I also don't think it's helpful to file an issue as "look, a nail: here's why you should buy my hammer". It's possible to agree on the problem, without agreeing on the solution; in this case, eg, I think the identified problems can be effectively solved without all the refactoring, in particular via the approach in #35187.

Concept ACK

I think its worth reiterating we are our own "third party." It will be necessary and desirable to refactor or change completely certain parts of our codebase now and in the future. Having consensus entangled with non-consensus implementation details as it is today makes this risky, and and far more work than should be necessary.

Testing is the best example of this. If consensus can only be tested through non-consensus implementation details, you necessarily must change the tests along with implementation changes you want to make. Ideally, we get to a point where the stateless consensus is fully tested, and we can change non-consensus details, such as state management or the execution / orchestration of consensus, with a guarantee that the actual consensus boundary didn't change. There are of course other benefits, such as easier fault injection, and passing in unlikely or almost impossible scenarios to ensure they are handled correctly, without needing to go through a bunch of other code that might not allow those degrees of freedom.

I also think this should have big implications for fuzz testing, and for differential testing against other consensus implementations which don't use the same data model. Doing so helps find things in our own implementation that can / should be fixed and also improves the health of the entire ecosystem.

I think most people will not disagree on the benefits, but the point is: "Is the risk worth it?".

Refactors like this would require a ton of reviews (which means hours of reviewers time that could be directed to other PRs). I understand that we might end up at some point discussing a "chicken-and-egg" problem: "If we trust in our tests, we should not be afraid of refactoring. If we don't trust, maybe writing better tests would need some level of refactoring". However, I understand that this kind of change would not be just a simple refactor, it requires to rewrite/touch our tests (e.g. rewrite test code that mutates individual data members) which, in my opinion, further increases the risk.

As a result, unit tests require substantial test setup, and validation cannot be reused independently of the node's state-management implementation.

I agree that are unit tests are not elegant to read or write, and requiring substantial test setup isn't good. But I'm not sure about whether it would bring a significant speed up, do you have a benchmark/expectations on it? I strongly advocate in favor of having faster tests because it helps me on mutation testing, but tbh my main bottleneck is the functional tests.

Perhaps the description of the problem and the benefits presented in this issue are not convincing (maybe your blog posts have more details about them?). I'm not advocating in favor or against it but I could show some more points that could contribute to the discussion based on issues I got.

For fuzzing I think some benefits - that came to my mind - are:

1. Avoid the setup cost we have on some targets (e.g. utxo_total_supply).
2. Some determinism could get fixed at the root instead of patched.
3. It would allow to fuzz consensus rules deeply.
4. If it reduces lock annotations, so would reduce harness frictions (?)

For differential fuzzing/bitcoinfuzz, I think I had some issues on working on a differential contextual block validation which this kind of library would facilitate it (did not check if recent kernel PRs would also allow it btw).

After my comment on #35569 (comment), I kept digging at ways to enforce explicity ownership through construction for a stronger immutability guarantee and came up with something I think is interesting. It's more of a conceptual proposal which I'd like feedback on, so this seemed the appropriate place to mention it.

The idea is get rid of moves by forcing the constructor to read bytes through a span, but in a way that doesn't force extra copies in hot paths like reading transactions off the wire or when parsing a block:

<details><summary>transaction type with a privileged reader</summary>

class script {
    std::vector<std::byte> bytes_;

  public:
    script() = default;

    explicit script(std::span<const std::byte> bytes) : bytes_{bytes.begin(), bytes.end()} {}
    // const observers, equality friend etc
};

class input {
    std::uint32_t sequence_{0xffffffff};
    script script_sig_;

  public:
    input() = default;

    input(std::uint32_t sequence, script script_sig)
        : sequence_{sequence}, script_sig_{std::move(script_sig)} {}

    // const observers, equality friend etc
};

class transaction {
    struct body {
        std::vector<input> inputs;
        hash_id txid;

        explicit body(std::span<const input> source)
            : inputs{source.begin(), source.end()}, txid{hash(inputs)} {}

        explicit body(reader& source) : inputs{read(source)}, txid{hash(inputs)} {}

        static std::vector<input> read(reader& source) {
            std::vector<input> inputs;
            // consume bytes from the reader into inputs etc
            return inputs;
        }

        static hash_id hash(std::span<const input> inputs) {
            // do the hash
            return h;
        }
    };

    std::shared_ptr<const body> body_;

  public:
    transaction() : transaction{std::span<const input>{}} {}

    explicit transaction(std::span<const input> inputs)
        : body_{std::make_shared<const body>(inputs)} {}

    explicit transaction(reader& source) : body_{std::make_shared<const body>(source)} {}

    // all the const observers etc
    [[nodiscard]] std::span<const input> inputs() const { return body_->inputs; }
    [[nodiscard]] hash_id txid() const { return body_->txid; }

    // replacement for ctxref
    const transaction& operator*() const { return *this; }
    const transaction* operator->() const { return this; }

    // regular!
    friend bool operator==(const transaction& a, const transaction& b) {
    }
};
static_assert(std::regular<transaction>)

</details>

This makes it mandatory that the bytes are owned by the object, which gives us the strongest guarantee that the object is immutable after construction. This closes the alias/move issue I highlighted, where a buffer gets stolen via a move, but the elements in that buffer might have alias' or pointers that can be used to reach in and modify the elements.

There is also as constructor for a reader& object and read method on body enabling us to construct the object straight off a byte stream/deserialisation, which keeps this at parity with what we have today in hot paths like parsing a block for validation and reading transactions into the mempool. I have a minimal full implementation and benchmark which indicates this is correct, but obviously will want a full implementation before I can say definitively.

I think this is really nice. We construct the object once, get a strong guarantee of immutability because the class has no mutating methods with private members and fully owns its data as enforced by construction. Because its a regular type, we can pass around/copy the object for essentially nothing without needing any extra machinery (CTransactionRef, CMutableTransaction).

Thanks for the thoughtful response @brunoerg . I want to respond to a few of your points with a different view.

"Is this risk worth it?"

This question is typically posed under the assumption that the status quo is not risky. However, the biggest risks often come from not doing anything. The more interesting questions to me are "does the proposed change have benefits," "can we see a path to implementing this," and "what does this derisk in other areas, now and in the future." If a group of individuals agrees on the benefits, their main goal as a group of contributors and reviewers is to derisk the change collectively.

A change could take hundreds of hours of review, or it could not. That's part of derisking. A change could also take hundreds of hours today, to save 1,000 of hours of review and work in the future.

On tests

You mention elegant tests and fasts tests, but I think this is missing the point. Tests should be declarative and simple because thats how we know they are correct and testing the right thing. Speed is a matter of hardware. Tests that are difficult to read and that require substantial setup often contain bugs. If you are spending just as much time reviewing the test as you are reviewing the code, something is wrong. I think what is really meant here is: "we should be able to test validation directly, at its explicit boundary."

Furthermore, as I mentioned above, we want to get away from testing a specific implementation at a specific point in time that uses validation. Why? Because we will necessarily need to change the implementation at some point in the future and we want to have the strongest guarantees that changing the implementation does not change validation or lead to unexpected behaviours. We also will have to change validation at some point, and we will want a boundary that ensures validation was changed in a way we expect and the implementation consuming it was not.

Following up on my earlier suggestion for making CTransaction immutable with strong guarantees: had an offline conversation with @purpleKarrot and we realised the behaviour we are trying to protect against is in fact perfectly valid c++. There is a recommendation that std::move actually preserve pointers, refs, iterators etc, even after the move! Seems like people would want this for continued read only access? However, since its not UB to write through a reference to a non-const element, its also perfectly valid to mutate the element after it has been moved. I did some reading to see how other projects might be dealing with this, and I stumbled on https://chromium.googlesource.com/chromium/src/+/HEAD/docs/patterns/passkey.md. Basically, create a passkey class whose only purpose is to key a function so that it can only be accessed by someone who can construct the passkey. This is much nicer than declaring the decoder a friend of CTransaction because it doesn't allow access to any of the internals of the class; it only allows access to the specific keyed function. The end result looks really clean. Here's an updated version of my earlier sketch:

<details><summary>transaction with passkey on constructor</summary>

template <class T>
class pass_key {
    friend T;
    explicit pass_key() = default;
};

class decoder;

class transaction {
    struct body {
        std::vector<input> inputs;
        hash_id txid;

        explicit body(std::span<const input> source)
            : inputs{source.begin(), source.end()}, txid{hash(inputs)} {}

        explicit body(std::vector<input>&& source)
            : inputs{std::move(source)}, txid{hash(inputs)} {}

        static hash_id hash(std::span<const input> inputs) {
            // do the hash
            return h;
        }
    };

    std::shared_ptr<const body> body_;

  public:
    transaction() : transaction{std::span<const input>{}} {}

    explicit transaction(std::span<const input> inputs)
        : body_{std::make_shared<const body>(inputs)} {}

    // scoped by the key to the one caller
    // whose vectors are locals nobody else can name, doesnt
    // give access to the private vars like a friend would!
    explicit transaction(pass_key<decoder>, std::vector<input>&& inputs)
        : body_{std::make_shared<const body>(std::move(inputs))} {}

     // const observers etc
    [[nodiscard]] std::span<const input> inputs() const { return body_->inputs; }

    // replacement for ctxref
    const transaction& operator*() const { return *this; }
    const transaction* operator->() const { return this; }

    friend bool operator==(const transaction& a, const transaction& b) {
        return a.body_ == b.body_ || a.body_->txid == b.body_->txid;
    }
};

// deserialisation stays outside the vocabulary type, per std::bitcoin suggestion
// reads into locals, validates the full encoding, construct exactly once
class decoder {
  public:
    static std::optional<transaction> parse(reader& source) {
        std::vector<input> inputs;
        // consume bytes from the reader into inputs, any error returns nullopt,
        // and not an empty transaction
        return transaction{pass_key<decoder>{}, std::move(inputs)};
    }
};

</details>

I'm fairly confident that with this we can make a strong guarantee of the immutability of CTransaction after construction, without unnecessarily hampering the performance or utility of the class, and retaining all of the benefits spelled out by @purpleKarrot by having the class be a regular type, not having exceptions on parsing, eliminating the "empty" or optional transaction case, etc.

EDIT: sauce on pointers staying valid after move: https://stackoverflow.com/questions/25347599/am-i-guaranteed-that-pointers-to-stdvector-elements-are-valid-after-the-vector

The problem can be generalised and summarised as:

Transferring ownership of an object while mutable aliases to its parts remain accessible opens a backdoor for breaking the owning class's invariants.

There are multiple ways to approach this. One way is to restrict ownership transfer. For CTransaction, it implies that its constructor should always copy the transaction inputs and outputs, or that a constructor that allows ownership transfer is only accessible with a passkey, as @josibake proposed above. I consider that approach as a patch and not a fix.

The fundamental problem is that it is possible to retain a mutable reference to a part of an object in the first place. It could be prohibited with a custom clang-tidy check that issues a diagnostic whenever the result of a member function is stored in a mutable reference.

I consider that approach as a patch and not a fix.

Summarising an offline conversation: this is good point. The problem is a generic problem, not a transaction object problem. If it is possible to write a clang-tidy plugin that catches the behaviour we want to disallow, this is better. It works for everything in the codebase out of the box, doesn't require having a special cased object, and more importantly doesn't require adding passkeys (or worrying about forgetting to add them to objects that should have them).

In the event the imagined clang tidy plugin is not possible, we can use the patch as a fallback. Thanks for the feedback!

This question is typically posed under the assumption that the status quo is not risky. However, the biggest risks often come from not doing anything.

I'd say the assumption is that the status quo is less risky. And the important thing is that this is subject to empiricism, especially when the status quo has existed for many years:

If you think the current way of doing things is risky, show the past bugs that could not have happened under the proposed changes. If there weren't any, show the past bugs in PRs that were found before merge during code review. If there weren't any either, show some past bugs detected during development. If there weren't any of these either, at least show plausible pitfalls devs could easily step into (but not artificial examples). That kind of list should give a good indication how risky the status quo is.

When it comes to consensus code refactors, I think it is not sufficient that there are "no technical argument arguments against" a change, that is just a necessary condition. It is a perfectly tenable position to say "yes, that's nicer, and we probably would do it that way if we'd write the code from scratch, but since it has been different for all that time without any apparent issues, the benefits don't outweigh the risks". That is the real point that needs to be argued against, and in my opinion the best way to do this is with empirical evidence.

I'd say the assumption is that the status quo is less risky.

Fair, should have said less. What I think you're referring to when you talk about the status quo existing for many years is operational risk: the current code/architecture has run and continues to run. That is evidence that the implementation works. It fails to address the risk of modifying validation, state management, and orchestration code while they remain tightly coupled. This is the risk I am concerned with. There is of course migration risk in separating them, and that should be evaluated, but it cannot be presumed without evidence to be greater than the ongoing risk of making future changes under the current architecture. We have evidence of severe bugs that can be attributed to the kind of coupling risk this proposal aims to reduce. Furthermore, past performance is evidence, but it is evidence for a different claim. It doesn't tell us anything about the bugs we haven't found yet. Refactoring clarity and better testing helps us find more bugs.

I disagree with treating past bugs that a new design would have prevented as the necessary or primary empirical test for a refactor, but since you asked, there are at least two relevant examples in my mind:

• CVE-2018-17144: an explicit duplicate input check was removed in reliance on behaviour in the UTXO processing path. A later redesign changed that behaviour, producing an inflation bug. That is an example of a change to state management unexpectedly changing the enforcement of a consensus rule
• The 2013 chain split provides another example: a Berkeley DB resource limit became an accidental and inconsistent block-validity rule. A storage layer failure leaked into what different nodes treated as a consensus result

Can I claim that the design here would have absolutely prevented those bugs? No. These examples demonstrate the exact class of failure being discussed: implementation details outside a clear validation boundary affecting which blocks are accepted as valid. Better separation would not make bugs impossible, but it would make consensus behaviour directly testable and make changes to state machinery and implementation details less able to change it implicitly.

So lets just never change anything! One reason I am interested in these types of refactors it is very likely Bitcoin will eventually need a soft fork to upgrade it to a PQ cryptography scheme. It is my strong belief this could require some rather invasive changes to the code, particularly around verification performance and the way validation work is executed. I'd like to do whatever we can now in preparation for that to make the code safer to modify, easier to test, and easier to review. The exact form of a future post quantum upgrade is not essential to the argument, rather one example of the kinds of future validation changes for which I want the strongest boundary and testing surface we can reasonably provide. It also doesn't have to be validation: it may be necessary to change our state management implementation. It works both ways.

When it comes to consensus code refactors, I think it is not sufficient that there are “no technical arguments against” a change.

I agree with that statement in isolation, but it is not an accurate characterisation of the argument here, or the argument being made on the linked PR. No one claimed “no one has produced a technical objection, therefore the change should be merged.” The linked statement came after an affirmative technical argument. it was not offered as a substitute for one.

I don't want to pull the discussion from the linked PR in here, so I'll only address what is claimed in this issue. The issue above makes an argument for removing validation from being tightly coupled with implementation details. There are also arguments for how a boundary aids testing, independent reuse, fault injection, fuzzing, differential testing, and safer changes to the implementation consuming validation, etc. Objections need to engage with the actual proposal, i.e. the problems and benefits listed above, rather than treating the current architecture as presumptively safer simply because it is the current architecture.