refactor: extract KeyValueStore to common #3621

isegall-da · 2026-01-20T21:04:17Z

This factors out of the validator, to common, the key-value storage functionality from ValidatorInternalStore, and also applies some renaming to make it a more generic key-value and not specifically for "configurations".

Motivation:
Add a key-value store to Scan (specifically, in the near-term, to store progress of the bulk-storage dumps)

Pull Request Checklist

Cluster Testing

If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

Include any change that might be observable by our partners or affect their deployment in the release notes.
Specify fixed issues with Fixes #n, and mention issues worked on using #n
Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/KeyValueStore.scala

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

isegall-da · 2026-01-21T01:24:01Z

.../src/main/scala/org/lfdecentralizedtrust/splice/validator/store/ValidatorInternalStore.scala

I'm thinking now that maybe this should just be added as another store in DbAppStore instead of having this class (and a similar one in Scan very soon). Thoughts?

I don't understand the suggestion, you want to add a generic def keyValues: KeyValueStore to trait AppStore?

Something like that. Mostly noticed that all other instantiations of StoreDescriptor are all in one place (e.g. for scan, it's in DbScanStore), so was wondering whether we want to add it somewhere that each app just needs to define the StoreDescriptor for it. But that whole setup with AcsStores, TxLogStores, etc is so complex as it is, that I'm no longer thinking this is a great idea...

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

nicu-da

Nice, thanks! This makes me wonder how do we treat such releases that make rollbacks not possible without a restore from backup though 🤔 Should we add a note to the release notes?

ray-roestenburg-da · 2026-01-21T12:04:33Z

apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/DbKeyValueStore.scala

    val jsonValue: Json = value.asJson

-    val action = sql"""INSERT INTO validator_internal_config (config_key, config_value, store_id)
+    val action = sql"""INSERT INTO key_value_store (key, value, store_id)


changing the table name, what about existing data? (does this matter for the validator store version?)

We're only renaming a table and some columns/constraints, that shouldn't affect existing data, no?

The table itself is also renamed accordingly in flyway, so data would be preserved

thanks missed that (flyway)

ray-roestenburg-da

Thanks

rautenrieth-da

Thanks!

rautenrieth-da · 2026-01-21T13:33:04Z

apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/DbKeyValueStore.scala

    val jsonValue: Json = value.asJson

-    val action = sql"""INSERT INTO validator_internal_config (config_key, config_value, store_id)
+    val action = sql"""INSERT INTO key_value_store (key, value, store_id)


We're only renaming a table and some columns/constraints, that shouldn't affect existing data, no?

rautenrieth-da · 2026-01-21T13:48:06Z

apps/common/src/test/scala/org/lfdecentralizedtrust/splice/store/KeyValueStoreTest.scala

+    val otherKey = "key2"
+    val otherValue = "jsonPayload2"
+
+    "set and get a payload successfully" in {


Suggestion for one more test: create two stores with different descriptors, check that they don't affect each others data.

👍
(note that these tests are not new, it's just refactoring of the existing ones, separating the generic key-store ones from the ones specific for validator data, but I'll add a test nevertheless)

rautenrieth-da · 2026-01-21T13:52:55Z

.../src/main/scala/org/lfdecentralizedtrust/splice/validator/store/ValidatorInternalStore.scala

I don't understand the suggestion, you want to add a generic def keyValues: KeyValueStore to trait AppStore?

rautenrieth-da · 2026-01-21T14:04:53Z

This makes me wonder how do we treat such releases that make rollbacks not possible without a restore from backup though

Doesn't that apply to any change that adds a new flyway migration? We have 50 migrations already and haven't called them out in the release notes before. Shouldn't we just make it the default expectation that you can't downgrade to a previous release without restoring from backup?

nicu-da · 2026-01-21T14:08:29Z

This makes me wonder how do we treat such releases that make rollbacks not possible without a restore from backup though

Doesn't that apply to any change that adds a new flyway migration? We have 50 migrations already and haven't called them out in the release notes before. Shouldn't we just make it the default expectation that you can't downgrade to a previous release without restoring from backup?

Additions are not an issue for rollbacks, and i suspect most of our migrations are additions and not mutations/deletions. We have occasionally rolled back at least on our own nodes so I think we should start giving more awareness to these forward only changes.

rautenrieth-da · 2026-01-21T14:49:41Z

Additions are not an issue for rollbacks

Hm, this line says we explicitly configure flyway to complain if the database contains future migrations (those that are applied but not yet known to the running application). Is that not preventing application startup?

If not then mentioning database schema changes in release notes could be useful. Maybe a general compatibility table with columns like (CN release version, CN database version, Canton version, Canton database/protocol version)?

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

isegall-da · 2026-01-21T19:42:05Z

Hm, this line says we explicitly configure flyway to complain if the database contains future migrations

So we basically never allowed downgrade between versions that contain any flyway migration?

nicu-da · 2026-01-22T08:03:06Z

Hm, this line says we explicitly configure flyway to complain if the database contains future migrations

So we basically never allowed downgrade between versions that contain any flyway migration?

It does look like it. Not ideal

isegall-da added 2 commits January 20, 2026 21:03

[ci] refactor: extract KeyValueStore to common

66d983c

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

[ci] fmt

1a760b5

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

isegall-da commented Jan 20, 2026

View reviewed changes

apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/KeyValueStore.scala Outdated Show resolved Hide resolved

isegall-da added 4 commits January 20, 2026 22:04

[ci] refactor the test

265d086

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

[ci] rename the table

948ebf3

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

[ci] copyright

06f4797

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

[ci] cleanup

b4ebfbd

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

isegall-da requested review from nicu-da, pasindutennage-da, rautenrieth-da and ray-roestenburg-da January 20, 2026 22:36

isegall-da commented Jan 21, 2026

View reviewed changes

isegall-da added 2 commits January 21, 2026 01:41

[ci] testConfig

af61f0e

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

[ci] fix cleanup

fc9fb8d

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

nicu-da approved these changes Jan 21, 2026

View reviewed changes

ray-roestenburg-da reviewed Jan 21, 2026

View reviewed changes

ray-roestenburg-da approved these changes Jan 21, 2026

View reviewed changes

rautenrieth-da approved these changes Jan 21, 2026

View reviewed changes

[ci] another test

a4ff950

Signed-off-by: Itai Segall <itai.segall@digitalasset.com>

isegall-da merged commit ba16a10 into main Jan 23, 2026
62 checks passed

isegall-da deleted the isegall/kvstore branch January 23, 2026 21:44

refactor: extract KeyValueStore to common #3621

refactor: extract KeyValueStore to common #3621

Uh oh!

Conversation

isegall-da commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Cluster Testing

PR Guidelines

Merge Guidelines

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicu-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ray-roestenburg-da left a comment

Choose a reason for hiding this comment

Uh oh!

rautenrieth-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rautenrieth-da commented Jan 21, 2026

Uh oh!

nicu-da commented Jan 21, 2026

Uh oh!

rautenrieth-da commented Jan 21, 2026

Uh oh!

isegall-da commented Jan 21, 2026

Uh oh!

nicu-da commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

isegall-da commented Jan 20, 2026 •

edited

Loading

nicu-da commented Jan 22, 2026 •

edited

Loading