Add logger for VSS by tankyleo · Pull Request #87 · lightningdevkit/vss-server

tankyleo · 2026-01-22T08:04:27Z

This is a clone of the ldk-server logger

ldk-reviews-bot · 2026-01-22T08:04:30Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

This is a clone of the `ldk-server` logger

tankyleo · 2026-01-22T08:16:04Z

Based on #86 thought you'd want to take a look tnull given your past debugging sessions against VSS.

tnull · 2026-01-22T10:14:22Z

rust/impls/src/postgres_store.rs

 	tokio::spawn(async move {
 		if let Err(e) = connection.await {
-			eprintln!("Connection error: {}", e);
+			warn!("Connection error: {}", e);


Is this recoverable? error then?

We can recover using fn ensure_connected if this happens on one of our 10 long-lived connections to the database.

let me know if you would rather use error here it is true that if this happens during startup we do not recover.

In case we fail during startup we do have the error!("Failed to start postgres backend"); messages at the error level.

Yeah, I think error would be preferable in this case here, too.

Hmm I'm not quite fully onboard yet can you explain further ? Currently I don't want to add an error log after launching the main service loop for something we can recover from.

Ah, I interpreted

it is true that if this happens during startup we do not recover.

to agree that we can't recover from it period, but you're saying we only won't recover if it's hit during startup. Fine by me then.

Yes sounds good ok.

tnull · 2026-01-22T10:14:41Z

rust/impls/Cargo.toml

 tokio = { version = "1.38.0", default-features = false, features = ["rt", "macros"] }
 native-tls = { version = "0.2.14", default-features = false }
 postgres-native-tls = { version = "0.5.2", default-features = false, features = ["runtime"] }
+log = "0.4.29"


Disable default features?

tnull · 2026-01-22T10:15:32Z

rust/server/src/main.rs

 			auth
 		} else {
-			println!("No authentication method configured, all storage with the same store id will be commingled.");
+			warn!("No authentication method configured, all storage with the same store id will be commingled.");


I do start to wonder if we should only expose this behind a testing feature or cfg flag?

Now only available via cargo run --no-default-features --features testing see below

Could you expand why a feature is preferable to a cfg flag here? Features are meant to be user-faced, while cfg flag is really more development-oriented, and I'm leaning that NoopAuthorizer is rather the latter, i.e., should only ever be used by us / our unit tests, maybe. Everybody else should even be testing against whatever auth impl they run in production?

Thanks for the context switched to the cfg flag below

tnull · 2026-01-22T10:16:44Z

rust/server/src/vss_service.rs

 	store: Arc<dyn KvStore>, user_token: String, request: GetObjectRequest,
 ) -> Result<GetObjectResponse, VssError> {
-	store.get(user_token, request).await
+	trace!("handling GetObject request with store_id {}", request.store_id);


Is it worth logging truncated key values to give an indication what is being written/read?

I want to be careful with logging what could be sensitive information. Won't the keys all be obfuscated anyway via SorableBuilder in LDK-Node ? If so an operator of vss-server wouldn't get much visibility into what is getting stored.

I want to be careful with logging what could be sensitive information. Won't the keys all be obfuscated anyway via SorableBuilder in LDK-Node ?

Yes, they will be obfuscated and hence logging truncated keys should be fine. It will allow to somewhat correlate with the client side if we need to debug something though.

Sounds good I added the same logging as vss-client below, and chose to log the full keys instead of the truncated keys since these are obfuscated, and want to maintain consistent logs between vss-client and vss-server.

ldk-reviews-bot · 2026-01-26T00:00:33Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

LGTM, I think. Feel free to squash the fixups.

tnull · 2026-01-26T08:30:04Z

rust/impls/src/postgres_store.rs

 	tokio::spawn(async move {
 		if let Err(e) = connection.await {
-			eprintln!("Connection error: {}", e);
+			warn!("Connection error: {}", e);


Ah, I interpreted

it is true that if this happens during startup we do not recover.

to agree that we can't recover from it period, but you're saying we only won't recover if it's hit during startup. Fine by me then.

`NoopAuthorizer` is now only accessible via `RUSTFLAGS="--cfg noop_authorizer" cargo run --no-default-features`. We want to discourage our users from ever using `NoopAuthorizer`.

tankyleo requested a review from tnull January 22, 2026 08:05

Add a logger for VSS

82b9211

This is a clone of the `ldk-server` logger

tankyleo force-pushed the 26-01-logging branch from b2bd20a to 7c84099 Compare January 22, 2026 08:06

tnull reviewed Jan 22, 2026

View reviewed changes

tankyleo self-assigned this Jan 22, 2026

tankyleo added this to Weekly Goals Jan 22, 2026

tankyleo moved this to Goal: Merge in Weekly Goals Jan 22, 2026

tankyleo requested a review from tnull January 22, 2026 23:41

tnull removed their request for review January 23, 2026 08:19

tankyleo force-pushed the 26-01-logging branch from a595901 to d4eae63 Compare January 23, 2026 22:05

tankyleo requested a review from tnull January 23, 2026 22:12

tankyleo force-pushed the 26-01-logging branch 6 times, most recently from f36256d to 833501a Compare January 24, 2026 00:00

tnull reviewed Jan 26, 2026

View reviewed changes

tankyleo added 2 commits January 26, 2026 17:56

Move all println's after logger setup to the log facade

c8f7959

Gate NoopAuthorizer behind the noop_authorizer cfg flag

52fd919

`NoopAuthorizer` is now only accessible via `RUSTFLAGS="--cfg noop_authorizer" cargo run --no-default-features`. We want to discourage our users from ever using `NoopAuthorizer`.

tankyleo force-pushed the 26-01-logging branch from 833501a to 52fd919 Compare January 26, 2026 17:57

tankyleo requested a review from tnull January 26, 2026 18:03

tnull approved these changes Jan 26, 2026

View reviewed changes

tnull merged commit 870f23c into lightningdevkit:main Jan 26, 2026
6 checks passed

github-project-automation bot moved this from Goal: Merge to Done in Weekly Goals Jan 26, 2026

tankyleo deleted the 26-01-logging branch January 26, 2026 19:49

tankyleo mentioned this pull request Jan 26, 2026

Add logging to the Rust vss-server #78

Closed

Conversation

tankyleo commented Jan 22, 2026

Uh oh!

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

Uh oh!

Uh oh!

tankyleo commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jan 26, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Jan 22, 2026 •

edited

Loading

tankyleo Jan 22, 2026 •

edited

Loading

tankyleo Jan 22, 2026 •

edited

Loading

tankyleo Jan 23, 2026 •

edited

Loading