Repository navigation
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
tnull
marked this pull request as draft
August 22, 2025 09:14
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
August 22, 2025 09:53
ac8ae88 to
3490f2a
Compare
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
2 times, most recently
from
November 6, 2025 11:41
bec8829 to
e977a6d
Compare
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
3 times, most recently
from
November 6, 2025 11:57
c7fc423 to
dd9a9e0
Compare
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
6 times, most recently
from
November 10, 2025 15:48
96db24d to
e248dc5
Compare
8 tasks done
tankyleo
self-requested a review
November 10, 2025 21:59
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
2 times, most recently
from
November 12, 2025 10:51
70f30cc to
85fd473
Compare
Collaborator
Author
|
Oh, seems when we also obfuscate the namespaces we might end up with keys that are too long. Will need to get back to the drawing board once more. |
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 12, 2025 19:10
173bfbc to
c84a361
Compare
tankyleo
reviewed
Nov 14, 2025
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 14, 2025 09:02
bc2b4e5 to
7d78977
Compare
tnull
commented
Nov 14, 2025
tnull
left a comment
tnull
left a comment
Collaborator
Author
There was a problem hiding this comment.
Addressed pending feedback
tankyleo
reviewed
Nov 14, 2025
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 15, 2025 17:58
7d78977 to
867f0ec
Compare
We introduce an `enum VssSchemaVersion` that will allow us to discern different behaviors based on the schema version based on a backwards compatible manner.
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 17, 2025 15:07
867f0ec to
040dcf1
Compare
Collaborator
Author
|
Rebased after #689 has been merged. |
tankyleo
reviewed
Nov 17, 2025
tankyleo
left a comment
tankyleo
left a comment
Contributor
There was a problem hiding this comment.
reviewed the rebase, good to squash
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 17, 2025 18:43
040dcf1 to
94318be
Compare
Collaborator
Author
Squashed without further changes. |
We bump our `vss-client` dependency to include the changes to the `StorableBuilder` interface. Previously, we the `vss-client` didn't allow to set `ChaCha20Poly1305RFC`'s `aad` field, which had the `tag` not commit to any particular key. This would allow a malicious VSS provider to substitute blobs stored under a different key without the client noticing. Here, we now set the `aad` field to the key under which the `Storable` will be stored, ensuring that the retrieved data was originally stored under the key we expected, if `VssSchemaVersion::V1` is set. We also now obfuscate primary and secondary namespaces in the persisted keys, if `VssSchemaVersion::V1` is set. We also account for `StorableBuilder` now taking `data_decryption_key` by reference on `build`/`deconstruct`.
While having it in `VssStoreInner` makes more sense, we now opt to construt the client (soon, clients) in `VssStore` and then hand it down to `VssStoreInner`. That will allow us to use the client once for checking the schema version before actually instantiating `VssStoreInner`.
We previously attempted to drop the internal runtime from `VssStore`, resulting into blocking behavior. While we recently made changes that improved our situation (having VSS CI pass again pretty reliably), we just ran into yet another case where the VSS CI hung (cf. https://github.com/lightningdevkit/vss-server/actions/runs/19023212819/job/54322173817?pr=59). Here we attempt to restore even more of the original pre- ab3d78d / lightningdevkit#623 behavior to get rid of the reappearing blocking behavior, i.e., only use the internal runtime in `VssStore`.
Now that we rely on `reqwest` v0.12.* retry logic as well as client-side timeouts, we can address the remaining TODOs here and simply drop the redundant `tokio::timeout`s we previously added as a safeguard to blocking tasks (even though in the worst cases we saw they never actually fired).
To avoid any blocking cross-runtime behavior that could arise from reusing a single client's TCP connections in different runtime contexts, we here split out the `VssStore` behavior to use one dedicated `VssClient` per context. I.e., we're now using two connections/connection pools and make sure only the `blocking_client` is used in `KVStoreSync` contexts, and `async_client` in `KVStore` contexts.
Since we just made some breaking changes to how exactly we persist data via VSS (now using an `aad` that commits to the key and also obfuscating namespaces), we have to detect which schema version we're on to ensure backwards compatibility. To this end, we here start reading a persisted `vss_schema_version` key in `VssStore::new`. If it is present, we just return the encoded value (right now that can only be V1). If it is not present, it can either mean we run for the first time *or* we're on V0, which we determine checking if anything related to the `bdk_wallet` descriptors are present in the store. If we're running for the first time, we also persist the schema version to save us these rather inefficient steps on following startups.
We add a test case that ensures that a node started and persisted on LDK Node v0.6.2 can still be successfully started with the new schema changes. Co-authored by Claude AI
This is close to the backwards compatibility test we just added for v0, now just making sure we can actually read the data we persisted with our current (V1+) code. Co-authored by Claude AI
tnull
force-pushed
the
2025-08-upgrade-vss-encryption-obfuscation-scheme
branch
from
November 17, 2025 18:45
94318be to
d2153f2
Compare
tankyleo
approved these changes
Nov 17, 2025
1 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on lightningdevkit/vss-client#40
In this PR we account for
vss-clientbeing renamed tovss-client-ng, upgrade to the latest v0.4.0 release, as well as fix minor issues with the data encryption and key obfuscation scheme currently employed byVssStore.As these are breaking changes, we'll also include a migration procedure as part of this PR.