Skip to content

go/worker/registration: Clean dead registration signer code#6481

Merged
martintomazic merged 6 commits intomasterfrom
martin/trivial/registration-worker-signer
Apr 7, 2026
Merged

go/worker/registration: Clean dead registration signer code#6481
martintomazic merged 6 commits intomasterfrom
martin/trivial/registration-worker-signer

Conversation

@martintomazic
Copy link
Copy Markdown
Contributor

@martintomazic martintomazic commented Mar 25, 2026

Motivation: Simplify #6474 (comment).

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 25, 2026

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 573fb3c
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-oasis-core/deploys/69cfc87d5783ce0009306213

Comment thread go/worker/registration/worker.go Outdated
@martintomazic martintomazic force-pushed the martin/trivial/registration-worker-signer branch 2 times, most recently from f596604 to 825920c Compare March 25, 2026 09:24
Comment thread go/worker/registration/worker.go
@martintomazic martintomazic changed the title go/worker/registration: Use node signed registrations for tests also go/worker/registration: Clean dead registration signer code Mar 25, 2026
@martintomazic martintomazic force-pushed the martin/trivial/registration-worker-signer branch from 825920c to e001aff Compare March 25, 2026 10:24
@martintomazic martintomazic marked this pull request as ready for review March 25, 2026 13:43
@martintomazic martintomazic force-pushed the martin/trivial/registration-worker-signer branch from f37acc8 to 7e2a103 Compare March 26, 2026 10:19
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 78.12500% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.01%. Comparing base (fcf368b) to head (573fb3c).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
go/oasis-node/cmd/node/node.go 58.33% 5 Missing and 5 partials ⚠️
go/worker/registration/config/config.go 71.42% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6481      +/-   ##
==========================================
- Coverage   64.45%   64.01%   -0.44%     
==========================================
  Files         699      698       -1     
  Lines       68216    68195      -21     
==========================================
- Hits        43966    43653     -313     
- Misses      19187    19521     +334     
+ Partials     5063     5021      -42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@peternose peternose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 😉

Comment thread go/worker/registration/config/config.go Outdated
Comment thread go/worker/registration/config/config.go Outdated
Comment thread go/oasis-node/cmd/debug/byzantine/node.go Outdated
Comment thread go/oasis-node/cmd/node/node.go Outdated
Comment thread go/worker/registration/worker.go Outdated
w.storedDeregister = storedDeregister

// If node has no entity it will not register.
if entityID != nil {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the best, as looking at the constructor signature one has no idea that this can be nil. Can we add another commit and fix TODO below (stop abusing no-op workers)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the best, as looking at the constructor signature one has no idea that this can be nil

Go type system is explicit. The current constructor signature means: entity id may be nil or have value of type public key. Pointer types are not so nice workaround for the missing option type in go. You use them to avoid copying huge values, have a shared reference, or mimic optional type.

What is not best is to assume that a pointer is not nil unless documented explicitly. We do this too often, with immutable small structs (smell), that should not be passed as pointers in the first place.

I agree this is still ugly, thus I have added an explicit comment. Clean way is to get rid of no-op workers but this is not trivial.

Comment thread go/oasis-node/cmd/node/node.go
Comment thread go/oasis-node/cmd/node/node.go
@martintomazic martintomazic force-pushed the martin/trivial/registration-worker-signer branch from 836120b to 68056b2 Compare April 3, 2026 08:58
@martintomazic martintomazic requested a review from peternose April 3, 2026 09:14
This is needed for introducing explicit configuration modes,
e.g. new observer mode and failing early in case of missing
entityID.

It also removes global state from the registration worker.
@martintomazic martintomazic force-pushed the martin/trivial/registration-worker-signer branch from 68056b2 to 573fb3c Compare April 3, 2026 14:02
@martintomazic martintomazic merged commit 1531f63 into master Apr 7, 2026
6 of 7 checks passed
@martintomazic martintomazic deleted the martin/trivial/registration-worker-signer branch April 7, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants