Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out what to do with EpochManager medium-term #6910

Open
matklad opened this issue May 30, 2022 · 4 comments
Open

Figure out what to do with EpochManager medium-term #6910

matklad opened this issue May 30, 2022 · 4 comments
Labels
C-housekeeping Category: Refactoring, cleanups, code quality C-tracking-issue Category: a tracking issue T-core Team: issues relevant to the core team

Comments

@matklad
Copy link
Contributor

matklad commented May 30, 2022

Today, we store EpochManager in NightshadeRuntie and access it via RuntimeAdapter.

This is awkward – EpochManager is very stateful, while runtime ideally shouldn't be. It also logically doesn't feel right – runtime should not care about evolution of epoch, it should exist within scope of one chunk.

The historical context is that we had plans to move EpochManager on chain (#2745).

I think we should just move EpochManager to chain:

  • epoch-manager-as-a-contract plans don't seem to materialize
  • even with epoch-manager-contrct, I think the Chain would want to maintain some kind of caches for epoch info. In genreal, I'd expect the interface for such smart contracts look like this:
trait RuntimeAdapter {
 fn run_builtin_contract(contract: AccountId, input: &[u8]) -> Result<Vec<u8>>;

that is, even if epoch manager lives on chain, runtime shouldn't know specifics of it, it should expose just a general hook to run arbitrary contract. It would be up to Chain to run that and interpret inputs/outputs as borsh-encoded epoch data.


There's additional, deep reason to move epoch manager to Chain – testing. Today, Chain is separated from Runtime via RuntimeAdapter. For testing purposes, it uses KeyValueRuntime, a simplistic runtime which can only process transfer transactions. However, as KeyValueRuntime lacks a true EpochManager, epoch-dependent tests have to be integration and have to use NightshadeRuntime.


Current status:

We now have EpochManagerAdapter which contains almost all of the stuff that EpcohManager does. At the moment, we still have RuntimeAdapter: EpochManagerAdapter. The next step is to remove that super-trait bound! that means that initially Chain would get a

struct Chain {
  runtime_adapter: Arc<dyn RuntimeAdapter>
  epoch_manager: Arc<dyn EpochManager>
}

which initially would point at the same object in memory.

@matklad matklad added C-housekeeping Category: Refactoring, cleanups, code quality T-core Team: issues relevant to the core team labels May 30, 2022
@matklad
Copy link
Contributor Author

matklad commented May 30, 2022

@matklad
Copy link
Contributor Author

matklad commented Jun 6, 2022

Note: while we probably want

struct Chain {
  em: EpochManager
}

this probably won't work right of the bat, as today we use separte chains but shared em for view client and client. So we should start with

struct Chain {
  em: Arc<Mutex<EpochManager>>,
}

Getting to end-state would require redesigning our client/view-client split.

@matklad
Copy link
Contributor Author

matklad commented Jun 14, 2022

More specific plan:

  1. Move SafeEpochManager type to some publicly visible place:

/// Wrapper type for epoch manager to get avoid implementing trait for foreign types.
pub struct SafeEpochManager(pub Arc<RwLock<EpochManager>>);
impl SafeEpochManager {
fn write(&self) -> RwLockWriteGuard<EpochManager> {
self.0.write().expect(POISONED_LOCK_ERR)
}
fn read(&self) -> RwLockReadGuard<EpochManager> {
self.0.read().expect(POISONED_LOCK_ERR)
}
}

  1. Specifically:
  • keep read/write methods, so that call-sites know that they are locking
  • keep it Arc/clone
  • make the .0 private
  • rename to something more reasonable. EpochManagerHandle?
  1. Add epoch_manager field to stuff which holds dyn RuntimeAdapter and initialize it to be the same object as the one in RuntimeAdapter. Notaly, Chain should be treated like this.

At this stage, we should reach the situation where runtime, view client and clinet all have separate epoch_manager field which point to the same actual object.
4. Slowly&Incrementally replace calls to RuntimeAdapter with calls to epoch_manager. Probably best done by removing one trait's method at a time.
5. Remove EpochManager from NightShadeRuntime.

At this point, we are done. Runtime (as in NighthsadeRuntime & RuntimeAdapter) doesn't have access to EpochManager. Clinet and ViewCLient share the same EpcohManager object, just like they used to before refactor
?. Perhaps at some point we need to introduce trait EpochManagerAdapter to break dependencies between crates. Perhaps not


Future steps:

  • revisit client tests -- perhaps manny can now work with KeyValue runtime?
  • Client&ViewClient sharing EpochManager feels suboptimal still. Figure out what to do there?

matklad added a commit to matklad/nearcore that referenced this issue Aug 17, 2022
The mechanical change here is rather small: rename SafeEpochManager to
EpochManagerHandle, and move it from nearcore to epoch_manager crate.

The architectural implications are rather large in contrast. What I aim
to do eventually is to remove EpochManager from RuntimeAdapter trait and
instead force everyone who needs em to hold EpochManagerHandle, rather
than dyn RuntimeAdapter. That way, the RuntimeAdapter would become
essentially stateless, and the shared mutable state of epoch manager
would be explicitly accounted for.

work towards near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 17, 2022
The mechanical change here is rather small: rename SafeEpochManager to
EpochManagerHandle, and move it from nearcore to epoch_manager crate.

The architectural implications are rather large in contrast. What I aim
to do eventually is to remove EpochManager from RuntimeAdapter trait and
instead force everyone who needs em to hold EpochManagerHandle, rather
than dyn RuntimeAdapter. That way, the RuntimeAdapter would become
essentially stateless, and the shared mutable state of epoch manager
would be explicitly accounted for.

work towards near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 17, 2022
The mechanical change here is rather small: rename SafeEpochManager to
EpochManagerHandle, and move it from nearcore to epoch_manager crate.

The architectural implications are rather large in contrast. What I aim
to do eventually is to remove EpochManager from RuntimeAdapter trait and
instead force everyone who needs em to hold EpochManagerHandle, rather
than dyn RuntimeAdapter. That way, the RuntimeAdapter would become
essentially stateless, and the shared mutable state of epoch manager
would be explicitly accounted for.

work towards near#6910
near-bulldozer bot pushed a commit that referenced this issue Aug 18, 2022
The mechanical change here is rather small: rename SafeEpochManager to
EpochManagerHandle, and move it from nearcore to epoch_manager crate.

The architectural implications are rather large in contrast. What I aim
to do eventually is to remove EpochManager from RuntimeAdapter trait and
instead force everyone who needs em to hold EpochManagerHandle, rather
than dyn RuntimeAdapter. That way, the RuntimeAdapter would become
essentially stateless, and the shared mutable state of epoch manager
would be explicitly accounted for.

work towards #6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
matklad added a commit to matklad/nearcore that referenced this issue Aug 18, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

context: near#6910
@matklad
Copy link
Contributor Author

matklad commented Aug 18, 2022

TODOs for potential follow up:

  • Split EpochManagerHandle into EpochManagerHandle and EpochManagerHandleMut. seems only chain wants to mutate EM
  • Merge BlockHeaderInfo and BlockInfo types, they only differ in rng seed it seems.
  • prev_hash/parent_hash/prev_block_hash should be consistent

near-bulldozer bot pushed a commit that referenced this issue Aug 30, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

Best reviewed by commit.

context: #6910
@matklad matklad added the C-tracking-issue Category: a tracking issue label Sep 20, 2022
matklad added a commit to matklad/nearcore that referenced this issue Sep 20, 2022
Part of near#6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.
matklad added a commit to matklad/nearcore that referenced this issue Sep 20, 2022
Part of near#6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.

You can see the rough idea of the plan in near#7606
matklad added a commit to matklad/nearcore that referenced this issue Sep 20, 2022
Part of near#6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.

You can see the rough idea of the plan in near#7606
matklad added a commit to matklad/nearcore that referenced this issue Sep 20, 2022
Part of near#6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.

You can see the rough idea of the plan in near#7606
matklad added a commit to matklad/nearcore that referenced this issue Sep 21, 2022
Part of near#6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.

You can see the rough idea of the plan in near#7606
matklad added a commit to matklad/nearcore that referenced this issue Nov 4, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 4, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 4, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 4, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 7, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 7, 2022
nikurt pushed a commit that referenced this issue Nov 9, 2022
We want `chain` to depend on epoch_manager, and, besides,
`ValidatorInfoIdentifier` is a very stupid reason to have such a dep.

Best reviewed by commit.

context: #6910
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910

The grand plan is to separate EpochManager out of NightshadeRuntime. As
a first step, I want to separate the *types* of runtime and epoch
manager, while keeping them in the same object. I want to do this via
the following series of steps:

```
trait RuntimeAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter: EpochManagerAdapter {}
```

```
trait EpochManagerAdapter {}

trait RuntimeAdapter {}
```

This PR introduces the EpochManagerAdapter infra.

You can see the rough idea of the plan in #7606
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910, depends on #7652

Must be reviewed per commit.
nikurt pushed a commit that referenced this issue Nov 9, 2022
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910, depends on #7656

This is slighly less trivial than pure move -- the API is adjusted to
remove direct dependency on doomslug which lives in a different crate
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910, depends on #7657

Must be reviewed per commit.
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910, depends on #7660

Must be reviewed per commit.
nikurt pushed a commit that referenced this issue Nov 9, 2022
Part of #6910, depends on #7665

Previously, this was only available on NightshadeRuntime, but seems fine
to expose everywhere.
nikurt pushed a commit that referenced this issue Nov 9, 2022
nikurt pushed a commit that referenced this issue Nov 9, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 10, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 10, 2022
matklad added a commit to matklad/nearcore that referenced this issue Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality C-tracking-issue Category: a tracking issue T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

1 participant