Move safety store to rust #982

siy · 2024-07-29T09:45:54Z

Summary

This PR moves Address Book and Safety Store to RocksDB (i.e. at Rust side of the Node code). The migration is of the existing stores is performed automatically at startup.

github-actions · 2024-07-29T09:46:34Z

Phylum Report Link

github-actions · 2024-07-29T11:08:13Z

Docker tags
docker.io/radixdlt/private-babylon-node:pr-982
docker.io/radixdlt/private-babylon-node:dc169feee0
docker.io/radixdlt/private-babylon-node:sha-dc169fe

codecov-commenter · 2024-07-29T11:15:53Z

Codecov Report

Attention: Patch coverage is 53.99325% with 409 lines in your changes missing coverage. Please review.

Project coverage is 43.5%. Comparing base (8d30f40) to head (4f621f7).

Files with missing lines	Patch %	Lines
core-rust/p2p/src/rocks_db.rs	0.0%	97 Missing ⚠️
...ore-rust/state-manager/src/jni/p2p/address_book.rs	0.0%	64 Missing ⚠️
...ore-rust/state-manager/src/jni/p2p/safety_store.rs	0.0%	44 Missing ⚠️
...ust/state-manager/src/jni/node_rust_environment.rs	0.0%	38 Missing ⚠️
...t/state-manager/src/jni/p2p/high_priority_peers.rs	0.0%	34 Missing ⚠️
core-rust/p2p/src/safety_store_components.rs	0.0%	17 Missing ⚠️
core-rust/node-common/src/store/rocks_db.rs	82.0%	16 Missing ⚠️
core-rust/p2p/src/components.rs	0.0%	14 Missing ⚠️
...src/main/java/com/radixdlt/safety/VertexIdDTO.java	23.5%	13 Missing ⚠️
...main/java/com/radixdlt/safety/BFTValidatorDTO.java	25.0%	9 Missing ⚠️
... and 22 more

Additional details and impacted files

@@             Coverage Diff             @@
##             develop    #982     +/-   ##
===========================================
+ Coverage       42.7%   43.5%   +0.7%     
- Complexity      4300    4571    +271     
===========================================
  Files           1695    1730     +35     
  Lines          52260   52998    +738     
  Branches        1503    1512      +9     
===========================================
+ Hits           22331   23068    +737     
+ Misses         29459   29453      -6     
- Partials         470     477      +7

Flag	Coverage Δ
rust	`43.5% <53.9%> (+0.7%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ain/java/com/radixdlt/p2p/AddressBookEntryDTO.java	`100.0% <100.0%> (ø)`
...idge/src/main/java/com/radixdlt/p2p/NodeIdDTO.java	`100.0% <100.0%> (ø)`
...java/com/radixdlt/p2p/RocksDbAddressBookStore.java	`100.0% <100.0%> (ø)`
...om/radixdlt/p2p/RocksDbHighPriorityPeersStore.java	`100.0% <100.0%> (ø)`
...in/java/com/radixdlt/safety/BFTValidatorIdDTO.java	`100.0% <100.0%> (ø)`
...n/java/com/radixdlt/safety/RocksDbSafetyStore.java	`100.0% <100.0%> (ø)`
...ge/src/main/java/com/radixdlt/safety/RoundDTO.java	`100.0% <100.0%> (ø)`
.../main/java/com/radixdlt/safety/SafetyStateDTO.java	`100.0% <100.0%> (ø)`
...rc/main/java/com/radixdlt/sbor/NodeSborCodecs.java	`99.2% <100.0%> (+99.2%)`	⬆️
...st/core-api-server/src/core_api/conversions/lts.rs	`0.0% <ø> (ø)`
... and 76 more

... and 78 files with indirect coverage changes

dhedey

Thanks Sergiy! For this review, I've just started with a high level / architectural review. Happy to do a more detailed review once the high level stuff feels correct.

In terms of the architecture, I think we need to move around the crate structure on the Rust side, and separate out the databases better -- I've captured more detailed thoughts in comments.

The code itself is looking nice though, we just want to make sure the architecture is right first.

dhedey · 2024-07-29T11:31:11Z

core/src/test-core/java/com/radixdlt/modules/FunctionalRadixNodeModule.java

@@ -316,7 +325,30 @@ public void configure() {

    switch (this.safetyRecoveryConfig) {
      case MOCKED -> install(new MockedSafetyStoreModule());
-      case BERKELEY_DB -> install(new BerkeleySafetyStoreModule());
+      case REAL -> install(


Shouldn't this now use Rocks / Rust, not Berkeley as the default test implementation?

dhedey · 2024-07-29T11:34:14Z

cli-tools/src/main/java/com/radixdlt/shell/RadixShell.java


      properties.set("db.location", dataDir.toString());
+      properties.set("db.location.node", new File(dataDir, "node").toString());


In my opinion we shouldn't be introducing explicit configuration for e.g. the db.location.node field - it should just be a fixed path under db.location. Having the layout fixed under db.location means we don't have to worry about people changing this when people build backup scripts.

Side note - if we're introducing a new config, it should be included in the env_subst file so it works in Docker.

Reworked to use the same config variable and fixed subdirectories under it.

dhedey · 2024-07-29T11:38:36Z

core-rust/state-manager/src/jni/node_rust_environment.rs

@@ -122,13 +125,16 @@ pub struct JNINodeRustEnvironment {
    pub state_manager: StateManager,
    pub metric_registry: Arc<Registry>,
    pub running_task_tracker: UntilDropTracker,
+    pub node_store: Arc<DbLock<ActualNodeDatabase>>,


A few things here:

I thought we were having separate databases for Safety and Address Book (and possibly migrations)?

If we did have one database, in my opinion we shouldn't call it the "node database" - it's a very misleading name as it doesn't include e.g. the ledger database.

I don't see why we need a lock around them here - this seems like it will be uselessly contentioned (particularly with a combined database, where e.g. a write of the address book would block reading the safety store) - Rocks does locking internally anyway where it needs to.

Perhaps this is a misunderstanding because I got the conclusion that these stores should be separate from the ledger, but otherwise it's fine to keep them together. But I see no problems separating them now.

Reworked to make databases separate.

dhedey · 2024-07-29T11:40:48Z

core/src/main/java/com/radixdlt/p2p/addressbook/BerkeleyAddressBookStore.java

@@ -101,7 +101,7 @@ public final class BerkeleyAddressBookStore implements AddressBookPersistence {
  public BerkeleyAddressBookStore(
      Serialization serialization,
      Metrics metrics,
-      @NodeStorageLocation String nodeStorageLocation,
+      @StateManagerStorageLocation String nodeStorageLocation,


I don't understand this - it looks like we're effectively moving the Berkeley address book here?

I'd assumed we only want this class around still for the purposes of migrating to the new address book store? So we should be using the old/existing location, so we can migrate from it?

This is to preserve the Berkeley address book location where it was before. It might be a bit misleading as the old NodeStorageLocation annotation is renamed to StateManagerStorageLocation and the new NodeStorageLocation annotation is now responsible for node-specific data - address book and safety state.

Ahh right - well as mentioned elsewhere, I think for now it's better to keep @NodeStorageLocation as the root directory, and just have each store use a different sub-directory from it :).

I want to avoid any node runners needing to add new configuration to take this update 👍

Restored old naming

dhedey · 2024-07-29T11:41:58Z

core-rust/state-manager/src/store/address_book_components.rs

+
+/// The ID of the node stored in the Address book (Secp256k1 public key)
+#[derive(Clone, Copy, Sbor)]
+pub struct AddressBookNodeId(pub [u8; AddressBookNodeId::LENGTH]);


This stuff shouldn't be in the state-manager crate as it's not relevant to the ledger / state manager database.

We may want to introduce new crates for consensus to stick the safety store, and p2p for the address book?

Where jni-export includes consensus, p2p etc.

We may then want to move some of the ledger helpers into node-common.

Makes sense. Will refactor these.

dhedey · 2024-07-29T11:49:56Z

core-rust/state-manager/src/store/address_book_components.rs

+#[derive(Clone, Sbor)]
+pub struct AddressBookEntry {
+    pub node_id: AddressBookNodeId,
+    pub banned_until: Option<i64>,


If that's a timestamp we probably want to create an EpochTimestamp new-type wrapping e.g. chrono's DateTime - with this to make it SBOR encode as an i64:

#[sbor( as_type = "i64", as_ref = "self.timestamp()", from_value = "Self(DateTime::from_timestamp(value, 0))" )]

Thanks for the hint.

After looking deeper, I realized that implementation must make assumptions about future use and convenient representation. Also, it requires adding a new dependency on crono just to make the code compile, there is no use of this representation on the Rust side for now. So, I've created a type alias and added a comment how this can be changed if necessary. Actual change, better to postpone until these timestamps will be actually used on the Rust side. Someone who will be implementing this part will be free to choose the most convenient underlying type without breaking binary compatibility.

Sounds fine / good to me :). We do have crono already on Core API - but I'm happy to push this back till later.

core-rust/state-manager/src/store/column_families.rs

dhedey · 2024-07-29T11:54:07Z

core-rust/state-manager/src/store/typed_cf_api.rs

@@ -738,6 +767,41 @@ impl DbCodec<()> for UnitDbCodec {
    }
 }

+/// A [`DbCodec]` capable of representing [`MigrationId]`.


Out of interest, why did we opt for manual codecs rather than SBOR encoding the id and migration status?

Good point. I've created them at the beginning, when I had a very basic understanding of how things done here. Will change to SBOR encoding.

…re working (address book is only partially tested)

dhedey

Nice work!

I haven't checked the DTO code -- ideally I'd like to see some tests that the migrations are working as expected, by writing to Berkeley, migrating, then reading from Rocks and comparing.

Separately, I think it'd be better in the long run to not have a migration store, and move the migration state into the individual stores; to avoid having to explain to node runners what this migration store is :)

dhedey · 2024-07-30T10:43:04Z

core/src/main/java/com/radixdlt/p2p/addressbook/BerkeleyAddressBookStore.java

@@ -101,7 +101,7 @@ public final class BerkeleyAddressBookStore implements AddressBookPersistence {
  public BerkeleyAddressBookStore(
      Serialization serialization,
      Metrics metrics,
-      @NodeStorageLocation String nodeStorageLocation,
+      @StateManagerStorageLocation String nodeStorageLocation,


Ahh right - well as mentioned elsewhere, I think for now it's better to keep @NodeStorageLocation as the root directory, and just have each store use a different sub-directory from it :).

I want to avoid any node runners needing to add new configuration to take this update 👍

dhedey · 2024-08-14T18:30:44Z

core-rust/state-manager/src/jni/node_rust_environment.rs

@@ -163,18 +165,51 @@ impl JNINodeRustEnvironment {

        let running_task_tracker = scheduler.into_task_tracker();

+        let address_book_db_path = Self::combine(&base_path, "address_book");
+        let safety_store_db_path = Self::combine(&base_path, "safety_store");


The Berkeley paths were address_book and consensus_safety_store.

Are we okay using the same paths? I guess Berkeley and Rocks don't share any file names; so we can just put them in the same directory; and the old Berkeley files will become irrelevant (but that's okay).

If we're okay with using the same paths, then we should use the same names:

address_book is good

safety_store needs to be renamed consensus_safety_store

dhedey · 2024-08-14T18:34:39Z

core/src/main/java/com/radixdlt/RadixNodeModule.java

@@ -391,7 +391,6 @@ protected void configure() {
            ScenariosExecutionConfig.resolveForNetwork(network)));

    // Recovery
-    install(new BerkeleySafetyStoreModule());


This file needs to be deleted I think?

Yes, it is. For some reason, git thinks that it was renamed into BFTValidatorDTO.

dhedey · 2024-08-14T18:41:30Z

core/src/main/java/com/radixdlt/rev2/modules/NodePersistenceModule.java

+
+  @Provides
+  @Singleton
+  BerkeleySafetyStateStore bdbSafetyStateStore(


To prevent any risk of confusion, I'd rather we don't provide the berkeley stores at all to guice, and just create them inline in the migration methods.

In fact, I'd suggest we don't need to provide any of these in guice, except the AddressBookPersistence and the PersistentSafetyStateStore

dhedey · 2024-08-14T18:42:01Z

core/src/main/java/com/radixdlt/rev2/modules/NodePersistenceModule.java

+
+  @Provides
+  @Singleton
+  RocksDbHighPriorityPeersStore rocksDbHighPriorityPeersStore(


Do we need this if the RocksDbAddressBookStore does both?

RocksDbAddressBookStore holds only the address book part. I wanted to keep all stores separate. On the Java side, putting it all together looked like an ad-hoc solution.

dhedey · 2024-08-14T19:02:57Z

core/src/test/java/com/radixdlt/rev2/RustMempoolTest.java

@@ -383,6 +302,51 @@ public void test_rust_mempool_getRelayTxns() throws Exception {
    }
  }

+  private NodeRustEnvironment createNodeRustEnvironment() throws IOException {


I wonder if it's worth moving this to some unit test helper or something? Given that we've duplicated it across a few tests now?

With the tests, we've been following the "Good Scout" rule and leaving our Java tests in a better shape than we found them every change.

Reworked tests to reuse common code. Unfortunately, due to Gradle issues it appeared too time-consuming to attempt to reuse test dependencies, and I ought to create two similar (but not identical) base classes for tests which require NodeRustEnvironment.

This is what core/src/test-core is for :). Create a new file NodeRustEnvironmentBuilder.java in core/src/test-core/java/com/radixdlt/rev2.

We've agreed as a team to use composition over inheritance for any new test utilities, so please don't introduce base classes :)

Such a location does not allow using it from core-rust-bridge, where 3 tests are located.

Reworked to have it as a utility class, rather than a base class.

dhedey · 2024-08-14T19:06:35Z

core/src/test/java/com/radixdlt/consensus/safety/RocksSafetyStateStoreTest.java

I can't see any tests of the migration?

I'd like to see a test where we:

Create a Berkeley store (e.g. just via a constructor) and commit various bits of data

Boot up a node. Wait for it to boot up. This should trigger the migration of the data.

Delete the data from the Berkeley store.

Check the state can be read from the node (ie that it must have migrated to Rust)

(for each of the stores).

Booting up the node can be done with a SimulationTest builder, setting a FunctionalRadixNodeModule set to use the same temporary folder for its database as the Berkeley store.

dhedey · 2024-08-14T19:10:45Z

core-rust/p2p/src/rocks_db.rs

+}
+
+/// A RocksDB-backed persistence layer for safety store.
+pub struct SafetyStoreDatabase<R> {


I'd expect the SafetyStore to be in a separate consensus crate. But I'm happy for us to move it at a later date if we just want to get this ticket in.

I think further refactoring (including moving p2p JNI part) should be done at the next stage, after discussion and making decision on final structure.

dhedey · 2024-08-14T19:13:46Z

core/src/test-core/java/com/radixdlt/modules/PrefixedNodeStorageLocationModule.java

@@ -86,7 +86,7 @@ public PrefixedNodeStorageLocationModule(String baseLocation) {
  @Provides
  @Singleton
  @NodeStorageLocation
-  private String nodeStorageLocation(@Self ECDSASecp256k1PublicKey publicKey) {
+  private String stateManagerStorageLocation(@Self ECDSASecp256k1PublicKey publicKey) {


I think this is still the nodeStorageLocation?

dhedey · 2024-08-14T19:14:59Z

core/src/main/java/com/radixdlt/consensus/safety/MigratingSafetyStore.java

+
+import java.util.Optional;
+
+public class MigratingSafetyStore implements PersistentSafetyStateStore {


I think this is unused / can be deleted? 👍

Removed file.

dhedey

Thanks Sergiy - it's looking a lot better, we're nearly there! Most of my comments can be left to a follow-up review, but the following should be done before merge - as they are core parts of the migration logic / end-state:

Changing the Cfs to be VersionedCfs over SBOR state (sorry I missed this before in such a large review)
Ensuring we don't instantiate Berkeley stores during bootstrapping unless we need to because we're not migrated.
Ensuring the address book migration test is complete (I think we're missing high priority peers at the moment)

dhedey · 2024-08-25T06:48:17Z

core-rust-bridge/src/test/java/com/radixdlt/helper/NodeRustEnvironmentBuilder.java

+    throw new IllegalStateException("Can't construct");
+  }
+
+  public static NodeRustEnvironment createNodeRustEnvironment(String dbPath) {


This isn't really a builder pattern... I'd expect something like: .new().dbPath(X).build()

But anyway, no need to change in this PR :)

The Builder pattern makes sense when there are no (or very few) mandatory parameters. In this case, there is only one (and mandatory) parameter. The Factory Method better suits in such a use case.

Renamed class to avoid confusion.

dhedey · 2024-08-25T06:54:55Z

core-rust/node-common/src/java/utils.rs

@@ -115,6 +115,21 @@ pub fn jni_sbor_coded_fallible_call<Args: ScryptoDecode, Response: ScryptoEncode
    .unwrap_or_else(std::ptr::null_mut)
 }

+/// A convenience method for the case when input should remain in the form of raw byte array
+/// (i.e. no SBOR decoding is needed). Output is still properly encoded with SBOR.
+pub fn jni_raw_sbor_fallible_call<Response: ScryptoEncode>(


Any reason we chose not to SBOR encode as a byte array? It's pretty cheap.

We used to have these kinds of raw methods, but they were confusingly inconsistent so I believe we got rid of them.

This is an SBOR encoded structure, and it is automatically SBOR-encode on the Java side. On the Rust side it has no use now and there is no sense decoding it because DB contains SBOR encoded struct anyway. There were two reasonable solutions

manually encode SBOR on the Java side, wrap it into array and when it comes back - manually unwrap and decode

get direct access to a byte array before it is decoded on the Rust side and pass it to the relevant underlying method

The second approach looked much simpler to me.

No longer an issue, and the corresponding helper is removed.

dhedey · 2024-08-25T06:56:14Z

core-rust/node-common/src/store/rocks_db.rs

Nice separation :)

dhedey · 2024-08-25T06:58:35Z

core-rust/node-common/src/store/typed_cf_api.rs

@@ -238,6 +238,14 @@ impl<'r, 'w, KC: BoundedDbCodec, CF: TypedCf<KeyCodec = KC>, R: WriteableRocks>
            self.cf.key_codec.upper_bound_encoding(),
        );
    }
+
+    /// Retrieves all entries.
+    pub fn get_all(&self) -> Vec<CF::Value> {


I don't think this necessarily needs to be put on the BoundedDbCodec impl. Perhaps it could be put on a more general impl? Or I imagine there's one there already?

We should also probably add to the Rust doc that this should only be used in prod when we know the total entries are bounded due to some kind of application limit?

I assume this is true in the case where we're using it?

Unfortunately, I found nothing like get_all. To me, it looked consistent to put it together with delete_all in the TypedCfApi implementation. It relies on the bounds in the key too, so the location looked about right.

Yes, this code could retrieve a lot of data, but it is used for address book, which rarely contains significant number of entries. There is no specific limit at the best of my knowledge, but the Java version has no limit either. So, if some kind of limit should be implemented, we need to discuss it first. Again, this is the reason to not put it in a more generic location as of now. Currently, it is used only in the AddressBook.

dhedey · 2024-08-25T07:01:04Z

core-rust/p2p/src/column_families.rs

+}
+
+/// Safety store
+pub struct SafetyStoreCf;


I think it would be nicer conceptually to put the safety store and address book in separate mods (both the store and relevant CFs). But I guess we'll be moving the safety store to a separate consensus crate next PR, so that's okay for now.

dhedey · 2024-08-25T07:39:30Z

core/src/test/java/com/radixdlt/p2p/test/P2PTestNetworkRunner.java

+        new AbstractModule() {
+          @Override
+          protected void configure() {
+            bind(AddressBookPersistence.class).to(BerkeleyAddressBookStore.class);


Why are we using Berkeley here instead of Rocks / the default?

Did Rocks break? This feels like a scary change to me - in that it might suggest this test breaks with the standard DB, and this was a work-around for a broken test :).

Either this test should be changed to use FunctionalNodeModule if it's an integration test; or if that's too big a change, at least at a comment here to explain we're hacking in the deprecated address Berkeley stores for now because it's too disruptive to add a node rust environment here.

I've tried to replace the implementation here, but got a very strange error:

com.google.inject.CreationException: Unable to create injector, see the following errors: 1) An exception was caught and reported. Message: called `Result::unwrap()` on an `Err` value: Error { message: "IO error: lock hold by current process, acquire time 1724758428 acquiring thread 6169243648: /var/folders/kq/xrwzyf4j3yq7pqnx_zkx9tym0000gn/T/junit5819788118170431666/state_manager/LOCK: No locks available" } at [unknown source] 2) [Guice/MissingConstructor]: No injectable constructor for type NodeRustEnvironment. class NodeRustEnvironment does not have a @Inject annotated constructor or a no-arg constructor. Requested by: 1 : NodeRustEnvironment.class(NodeRustEnvironment.java:75) at P2PTestNetworkRunner$1.configure(P2PTestNetworkRunner.java:198)

I'm going to add a comment and leave this part as is.

dhedey · 2024-08-25T07:42:34Z

core/src/test/java/com/radixdlt/p2p/addressbook/AddressBookMigrationTest.java

+public class AddressBookMigrationTest {
+  @Rule public TemporaryFolder folder = new TemporaryFolder();
+
+  private Module createCommonModule() {


We've had a rule to avoid creating Guice injectors for new tests, outside of FunctionalNodeModule - because they're very flakey to changes.

Why can't we just create the BerkeleyAddressBookStore in a private method in the test code without DI?

In fact, if we change the AddressBookModule to have a static ensureMigrated(rocksAddressBookStore, <config>) method, then we can just test that method here without creating any test injectors at all

dhedey · 2024-08-25T07:49:24Z

core/src/test/java/com/radixdlt/p2p/addressbook/AddressBookMigrationTest.java

+
+    // Create some entries in the Berkeley address book
+    try (var berkeleyAddressBook = injector.getInstance(BerkeleyAddressBookStore.class)) {
+      addresses.forEach(berkeleyAddressBook::upsertEntry);


Nice test - but I notice it's not complete? Can we also add some high priority peers?

dhedey · 2024-08-25T07:50:16Z

core/src/test/java/com/radixdlt/consensus/safety/SafetyStoreMigrationTest.java

+public class SafetyStoreMigrationTest {
+  @Rule public TemporaryFolder folder = new TemporaryFolder();
+
+  private Module createCommonModule() {


Similar to the other test - can we avoid adding new guice modules in tests? They create a maintenance burden

dhedey · 2024-08-25T07:51:55Z

core/src/test/java/com/radixdlt/consensus/safety/SafetyStoreMigrationTest.java

+    var injector =
+        Guice.createInjector(
+            createCommonModule(),
+            new NodePersistenceModule(AddressBookPolicy.DISABLED),


The address book persistence state should be irrelevant to this test - something feels off here

…ove-safety-store-to-rust

dhedey

Thanks Sergiy 👍

Happy for us to merge this, as any remaining comments are not related to how we persist things.

We can pick up any outstanding comments in a later PR which re-orgs the crates on the rust side.

dhedey · 2024-09-04T13:46:40Z

core-rust/p2p/src/safety_store_components.rs

+}
+
+#[derive(Debug, Clone, ScryptoCategorize, ScryptoEncode, ScryptoDecode)]
+pub struct LedgerHeader {


This duplicates LedgerHeader from StateManager, but that's fine for now. Perhaps we should add a comment to de-duplicate it / create some common models when we take out a consensus crate 👍

Added comment

dhedey · 2024-09-04T13:50:00Z

core-rust/p2p/src/components.rs

+    Debug, Clone, ScryptoCategorize, ScryptoEncode, ScryptoDecode, PartialOrd, Ord, PartialEq, Eq,
+)]
+#[sbor(transparent)]
+pub struct RawPublicKey(pub [u8; RawPublicKey::LENGTH]);


Minor - one to consider on a follow-up PR - perhaps we should call this a NodeSecp256k1PublicKey and be a wrapper around Secp256k1PublicKey from radix-common?

Renamed to NodeSecp256k1PublicKey. This clearly references the source where this data comes from and should later help with merging/wrapping existing type/etc.

dhedey · 2024-09-04T13:51:30Z

core-rust/p2p/src/components.rs

+
+/// The Secp256K1 signature
+#[derive(Debug, Clone, ScryptoCategorize, ScryptoEncode, ScryptoDecode)]
+pub struct Signature(pub [u8; Signature::LENGTH]);


One for a follow-up PR - we should use Secp256k1Signature from radix-common

Renamed to NodeSignature for the same reasons ad for NodeSecp256k1PublicKey.

dhedey · 2024-09-04T13:54:58Z

core-rust/state-manager/src/jni/node_rust_environment.rs

+
+    fn combine(base: &String, ext: &str) -> PathBuf {
+        let mut path = base.clone();
+        path.push(MAIN_SEPARATOR);


Minor - can be done in the follow-up PR - this should be done after converting to path buf, then you don't need to add a MAIN_SEPARATOR

Reworked. Realized that we can build PathBuf from the collection of strings.

dhedey · 2024-09-04T14:06:27Z

core/src/main/java/com/radixdlt/rev2/modules/REv2StateManagerModule.java

@@ -188,7 +191,8 @@ public static REv2StateManagerModule createForTesting(
        ledgerSyncLimitsConfig,
        protocolConfig,
        noFees,
-        scenariosExecutionConfig);
+        scenariosExecutionConfig,
+        false);


I can't see anywhere where FunctionalRadixNodeModule installs an AddressBookPersistence, directly or indirectly.

I believe what you're referring to is in the TestP2PModule which is installed in DeterministicNodes / SimulationTest. If necessary, feel free to add an option to TestP2PModule which allows using a real database.

But the following have no reason to live in a state manager module, so they should not be added here. And this state manager module should not have an installPersistence flag.

I really don't want more messy hacks to get some tests to work:

install(new PersistentSafetyStateStoreModule()); install(new AddressBookModule());

Please give me a ping if you'd like to pair on this and show me in person.

dhedey · 2024-09-04T14:11:10Z

core/src/test/java/com/radixdlt/p2p/test/P2PTestNetworkRunner.java

+
+          @Provides
+          @Singleton
+          BerkeleySafetyStateStore safetyStateStore(


Minor - do we need a BerkeleySafetyStateStore here?

Reworked module.

Signed-off-by: Sergiy Yevtushenko <[email protected]> Signed-off-by: Sergiy Yevtushenko <[email protected]>

sonarcloud · 2024-09-09T11:07:29Z

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
83.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

dhedey requested changes Jul 29, 2024

View reviewed changes

siy added 19 commits August 14, 2024 15:02

Intermediate commit: serialization, JNI, DB API.

a03e245

Intermediate commit: migration store, safety store and address book a…

7d7e992

…re working (address book is only partially tested)

Intermediate commit: all stores are working and tested

ef03a2b

Intermediate commit: address book implementation

641f31d

Intermediate commit: dependency injection issues, 8 tests failing.

e45fde8

Fix dependency injection in tests.

19524e9

Minor cleanup

b830d17

Mark each store as migrated.

9e55c3b

Remove unused class.

34f667f

Update properties files and make all default paths consistent

b961bf3

Separation of DB-related code at Rust side (step #1)

5ddbfa2

Separation of DB-related code at Rust side (step #2)

7171b4d

Separation of DB-related code at Rust side (step #3)

1f9b18e

Separation of DB-related code at Rust side (step #4)

6fcc859

Make separate address book, safety store and migration store.

0a31180

Use rocks db for tests.

1d988ad

Move common rocks db code to node-common

328039a

Move state-manager store files back to store module.

5fe0fcd

Move p2p files to dedicated crate.

8679a93

siy force-pushed the feature/NODE-609-move-safety-store-to-rust branch from 2284dab to 8679a93 Compare August 14, 2024 13:03

dhedey reviewed Aug 14, 2024

View reviewed changes

siy added 5 commits August 15, 2024 13:57

Address review comments.

a428d18

Let address book and safety store hold their migration status.

66c44a6

Remove base class from tests

fdf3a75

Fix conversion from DTO to preserve set ordering.

a41b6c9

Fix formatting

fe1fad4

siy added 3 commits August 21, 2024 13:57

Reuse NodePersistenceModule in FunctionalRadixNodeModule

25c13ef

Add migration tests

b99765b

Minor cleanup

bcc2576

dhedey reviewed Aug 25, 2024

View reviewed changes

siy added 12 commits August 27, 2024 13:56

Address review comments

1ba5812

Fix docker-related CI part

d3925ee

Fix docker-related CI part

fb4a814

Address Sonar issues

2eddb02

Attempt to fix Docker builds

464af48

Attempt to fix Docker builds

0ab42ea

Fix formatting

ffad413

Add versioning to DB data format.

9930783

Fix serialization for the NextEpoch.

d9c609d

Bump curl version to available one

6668337

Bump libssl-dev version to available one

3056773

Merge remote-tracking branch 'origin/develop' into feature/NODE-609-m…

feac88f

…ove-safety-store-to-rust

dhedey approved these changes Sep 4, 2024

View reviewed changes

Merge branch 'develop' into feature/NODE-609-move-safety-store-to-rust

d49e9b4

siy force-pushed the feature/NODE-609-move-safety-store-to-rust branch from e0cb7b4 to 5323631 Compare September 9, 2024 09:17

Address PR comments.

a2829da

Signed-off-by: Sergiy Yevtushenko <[email protected]> Signed-off-by: Sergiy Yevtushenko <[email protected]>

siy force-pushed the feature/NODE-609-move-safety-store-to-rust branch from 5323631 to a2829da Compare September 9, 2024 09:22

Bump libssl-dev version

4f621f7

siy merged commit d9d5c66 into develop Sep 13, 2024
22 checks passed

siy deleted the feature/NODE-609-move-safety-store-to-rust branch September 13, 2024 12:13


		properties.set("db.location", dataDir.toString());
		properties.set("db.location.node", new File(dataDir, "node").toString());


		import java.util.Optional;

		public class MigratingSafetyStore implements PersistentSafetyStateStore {

Move safety store to rust #982

Move safety store to rust #982

Conversation

siy commented Jul 29, 2024

Summary

github-actions bot commented Jul 29, 2024

github-actions bot commented Jul 29, 2024 • edited Loading

codecov-commenter commented Jul 29, 2024 • edited Loading

Codecov Report

dhedey left a comment

Choose a reason for hiding this comment

dhedey Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 29, 2024 •

edited

Loading

codecov-commenter commented Jul 29, 2024 •

edited

Loading

dhedey Jul 29, 2024 •

edited

Loading

dhedey Jul 29, 2024 •

edited

Loading

dhedey Jul 29, 2024 •

edited

Loading

dhedey Jul 29, 2024 •

edited

Loading

dhedey Aug 25, 2024 •

edited

Loading

dhedey Aug 25, 2024 •

edited

Loading