Ntx actor refactoring

[Mirko-von-Leipzig]

With #1628 largely complete, we can hopefully also simplify the actor side of the equation.

I touched on some ideas in #1314 (comment), but the foundations have since shifted a bit so lets consolidate our efforts in this issue. I'll jot my ideas down and maybe @SantiagoPittella can split them out into child issues as you see fit?

Overarching goals are to:

1. Limit number of network transactions being built.
2. Reap inactive network accounts.
3. Eliminate race conditions between actors and co-ordinator, and state updates.
4. Simplify and re-organize the code so its easier to understand.

(1) has already been solved using the semaphor + permit system, and I think that's quite effective.

Deactivating a network actor

I think there are two options.

1. Actor shuts itself down, and
2. co-ordinator kills the actor

(2) is already possible if we interpret closing the channel to the actor down. I think we just need to add logic to the actor that its an expected signal to receive, and not an error. However, this isn't important to implement yet because I don't think we have any need to do this from the coordinator side.

I'm mostly interested in implementing (1). The idea is simply that if an account's state is sterile for some amount of time, then that actor shuts down. Sterile meaning no viable transaction to create e.g. no possible notes to consume.

There is a race condition here where the coordinator sends a message, but the actor has shutdown. We can "detect" this by having the actor return its message channel on exit. The coordinator then receives this as part of the actor task ending, and can inspect it. If its empty, then the actor can stay dead, if its non-empty then maybe we should start the actor up again so it can handle these. If we don't do this then its possible for new network notes to be missed.

I think this solves the problem quite neatly, though maybe I've missed something :)

Simplifying coordinator -> actor messages

Since state is now all in the database, we no longer need to explicitly inform the actor about what changed. i.e. new notes would be queried from the database, account updates would be present as new transaction ID in the database etc.

This is still a fuzzy idea, but maybe its enough to just have a notification channel now? Where the actor can see if any changes have occurred and it should re-sync / check the database. Or maybe a different sync primitive, but I think you get the drift.

The downside of this is that constant database checks might be overly expensive? But we can "cheaply" query the account by checking the latest commitment (or transaction ID), and only fetching the update if its changed. It might be that sending deltas and applying them is still better.. but 🤷

It may also be a bad idea ito note churn -- maybe we already have unconsumed notes in-memory that we should retry before fetching more from the database. But I think maybe we should first try and then measure/profile to see if this is actually a problem ito performance.

The (very large imo) upside is that there is only a single source of truth -- the database, and database transactions let us avoid race conditions. Note that this also lets us handle new account transaction reverting -- because the account won't exist, so the actor can shutdown safely :)

Code organisation

No prescription here but we have a bunch of left-overs from my original non-actor impl. Sketch out a file layout that makes sense.

Account blacklisting

With things simplified we can add account blacklisting to protect against accounts/bugs that constantly crash. Keep a count of accounts that have crashed recently, and if they keep crashing then we simply don't start them up again. I would only tackle this once code layout and things are solid.

[SantiagoPittella]

• Deactivating actor

So, for this change and based on what you said about the race condtion I'm thinking of:

1. Actor sits in NoViableNotes for sterility_timeout (e.g. 5 min)
2. Actor sends ShutdownRequest { account_id, ack_tx } via shared mpsc channel
3. Builder main loop receives request
4. Coordinator removes actor from registry (no more events routed)
5. Coordinator sends () via ack_tx oneshot
6. Actor receives ack, returns ActorShutdownReason::Sterile(account_id)

That would need something like:

pub struct ShutdownRequest {
    pub account_id: NetworkAccountId,
    pub ack_tx: tokio::sync::oneshot::Sender<()>,
}

Plus the timeout configs and the message handling.

Also, should we spawn a new actor if the we receive a new transaction request for an already stopped actor?

• Simplifying coordinator -> actor messages

I like the tokio::sync::Notify approach, it will allow us to remove plenty of code 👍🏼 .

[Mirko-von-Leipzig]

said about the race condtion I'm thinking of:

We can actually make this even simpler imo. If the actor task ends then the coordinator also receives that task ending. And in that return we can pass back the notify channel for inspection.

[SantiagoPittella]

• Code Org

Yeah, we have many things that are a bit misplaced. We could move gRPC clients into its own folder. Also, the actors/ dir has a lot of DB related stuff. I can tackle this.

What do you think of this sub-issues?

1. Simplify the coordinator -> actor messages, to remove code.
2. Code organization, with the code removed we can think of a more "final" form for the structure before continuing to build on top.
3. Deactivating network actor.
4. Finally, account blacklisting.

[Mirko-von-Leipzig]

What do you think of this sub-issues?

I think that makes sense; ito (2) I would begin by sketching out the file/folder structure before working on the actual code. For example, I think having two separate database "clients" or APIs makes sense - one read-only for actor usage, and one writer for the coordinator.

[SantiagoPittella]

Okok, sounds good. I will start to work on (1) and the proposal of (2)

[SantiagoPittella]

To be addrssed as part of (1): #1689 (comment)

[SantiagoPittella]

For (2) I was thinking of something like the following, having the common types in src/, and then splitting by domain (actor, clients and db):

crates/ntx-builder/src/
├── lib.rs                    # Config, constants, build()
├── builder.rs                # NetworkTransactionBuilder event loop
├── coordinator.rs            # Coordinator, ActorHandle
├── chain_state.rs            # ChainState (extracted from builder.rs)
├── inflight_note.rs          # InflightNetworkNote + has_backoff_passed (moved from actor/)
│
├── actor/                    # Actor lifecycle and execution
│   ├── mod.rs                # AccountActor, ActorMode, ActorShutdownReason, ActorRequest, etc.
│   ├── candidate.rs          # TransactionCandidate struct + methods
│   └── execute.rs            # NtxContext, execution pipeline
│
├── clients/                  # External gRPC client wrappers
│   ├── mod.rs                # Re-exports `BlockProducerClient`, `StoreClient`
│   ├── block_producer.rs
│   └── store.rs
│
└── db/                       # Persistence layer, pretty much the same + account_effect
    ├── mod.rs
    ├── schema.rs
    ├── schema_hash.rs
    ├── migrations.rs
    └── models/
        ├── mod.rs
        ├── conv.rs
        ├── account_effect.rs # NetworkAccountEffect (moved from actor/)
        └── queries/
            ├── mod.rs
            ├── accounts.rs
            ├── notes.rs
            ├── note_scripts.rs
            ├── chain_state.rs
            └── tests.rs