wire go-synapse signer into keystore (vs go-filsigner), fix Wallet/Actor semantics by parkan · Pull Request #622 · data-preservation-programs/singularity

parkan · 2026-02-20T17:30:31Z

Problem

Singularity used go-filsigner for Filecoin message signing, with key material stored inline in the database (wallets.private_key column). The Wallet model was actually an on-chain actor identity (f0 address) with an embedded private key -- confusing naming that also coupled key management to database access and left a big security hole/private keys exposed in any db backup or access violation

For PDP deals, we need EVM transaction signing (secp256k1 ECDSA), which go-filsigner doesn't support. The existing model also couldn't represent the real relationship: a wallet (private key) exists before any on-chain actor, and PDP deals don't need an actor at all.

Solution

New keystore abstraction (util/keystore/):

KeyStore interface with Put/Get/List/Delete/Has — filesystem-backed, keys stored as lotus-export-format files named by address (for now)
Signer(ks, wallet) / EVMSigner(ks, wallet) — loads key from keystore, returns go-synapse Signer or EVMSigner
Replaces direct database reads for key material

Model rename — old Wallet becomes Actor:

The old Wallet struct (f0 actor ID + address + private key) is renamed to Actor — that's what it always was
Actor.PrivateKey column is orphaned (will be dropped by a future export-keys migration command)
GORM TableName() renames wallets → actors on auto-migrate

New Wallet model (model/wallet.go):

Represents a private key in an external keystore: KeyPath, KeyStore, Address
Optional ActorID foreign key — links to on-chain actor when one exists
Invariant: any deal we make requires a Wallet. Market deals also require an Actor (for ClientID). PDP deals need only a Wallet. Imported/tracked deals may have neither.

Preparation associates with Wallet, not Actor:

Preparation.Actors []Actor → Preparation.Wallets []Wallet (many2many wallet_assignments)
All handlers updated: attach/detach/listattached operate on Wallets
WalletChooser picks from []model.Wallet, DatacapWalletChooser dereferences wallet.ActorID for datacap lookup
Dealpusher market deal path: choose wallet → require ActorID → load actor → sign with wallet → pass actor to MakeDeal
Association to Actor can be loaded on demand, see below

MakeDeal takes ProposalSigner instead of db + keystore:

type ProposalSigner func([]byte) (*crypto.Signature, error)
Caller constructs the signer, MakeDeal just calls it — cleaner separation

Lazy actor resolution (handler/wallet/sign.go):

GetOrCreateActor() — on first market deal, queries Lotus for the wallet's on-chain actor, creates Actor record, links it
Supports the workflow: import wallet offline → fund externally → first deal auto-discovers actor

DealPusher options pattern (service/dealpusher/options.go):

WithPDPProofSetManager, WithPDPTransactionConfirmer, WithPDPSchedulingConfig, WithScheduleDealTypeResolver
Replaces growing constructor parameter list

Dependency changes

Added: go-synapse updated (b27d67ac9095) — provides signer.FromLotusExport, signer.Signer, signer.EVMSigner, signer.AsEVM
Removed: go-filsigner and its transitive deps (dchest/blake2b, drand/kyber, drand/kyber-bls12381)

What's NOT in this PR

Key export/migration command (exporting actors.private_key → keystore files) — separate PR
Dropping the private_key column — blocked on export command
PDP deal-making wiring (this PR provides the signing infra, actual PDP scheduling is Implement PDP schedule execution path and config wiring #621)

parkan · 2026-02-24T14:09:52Z

reviewing this next

parkan · 2026-02-24T14:24:37Z

taking lock on small fixes on this

… import code

Callers (dealpusher, send-manual handler) now resolve the wallet and build a signing closure before calling MakeDeal, keeping the deal maker free of keystore and database concerns.

parkan · 2026-02-24T16:38:46Z

this is definitely going to be a squash merge 😅

parkan · 2026-02-25T08:44:38Z

still tweaking this to clarify the wallet<>actor relationship but close

The model had Preparation associated with Actors (on-chain identities), but the correct invariant is: any deal we make requires a Wallet (private key), and a Wallet optionally has an Actor (for legacy market deals). PDP deals need only a Wallet. Imported/tracked deals may have neither. Core change: Preparation.Actors []Actor → Preparation.Wallets []Wallet with wallet_assignments join table (was actor_assignments with a TODO). Dealpusher market deal path now: choose wallet from Preparation.Wallets, require wallet.ActorID, load actor from DB, sign with wallet key, pass actor to MakeDeal. Previously it went through actors and reverse-looked up wallets. WalletChooser interface unchanged (already took []model.Wallet from prior commit) but DatacapWalletChooser now dereferences wallet.ActorID to find the on-chain client for datacap and pending deal queries. Handler changes: - attach/detach/listattached: operate on Preparation.Wallets directly, find wallet by address or uint ID, return []model.Wallet - schedule/create: preload Wallets, check len(preparation.Wallets) - dataprep/remove: cascade through Wallets association Swagger regenerated: ListAttachedWallets returns Wallet not Actor, ModelActor removed from generated client (unused). All tests updated: fixtures create Actor records for FK constraints (Deal.ClientID references actors table), use Wallets in Preparation fixtures, create wallets individually to avoid gorm batch uniqueIndex conflicts on KeyPath.

anjor · 2026-02-25T10:51:49Z

handler/wallet/remove.go

 	var affected int64
 	err := database.DoRetry(ctx, func() error {
-		tx := db.Where("address = ? OR id = ?", address, address).Delete(&model.Wallet{})
+		tx := db.Where("address = ? OR id = ?", address, address).Delete(&model.Actor{})


This changes /wallet/{address} removal semantics from deleting model.Wallet to deleting model.Actor.

That looks like an API regression: the wallet endpoint can return success while the wallet row (and keystore reference) remains, so wallet list still shows it. The swagger route/id are still wallet-scoped (RemoveWallet, /wallet/{address}).

Can we either:

keep this endpoint deleting model.Wallet (and decide key file behavior), or

rename/split endpoints so actor removal is explicit?

fair, we will eventually probably want actor-level deletions as well but will fix for correctness at this stage

anjor · 2026-02-25T10:54:37Z

handler/wallet/import_keystore.go

+		KeyStore: "local",
+		Address:  addr.String(),
+		Name:     request.Name,
+		ActorID:  nil, // populated lazily when needed


ActorID is intentionally nil here (offline import), but I don’t see the actor-linking flow wired into the runtime path yet.

As written, imported wallets can’t be used for market deals until something else sets ActorID.

yes this needs a lazy hydration once a message happens which due to invariants will always succeed (wallet funded or has datacap), I was going to wire that in later since the goal was to get this PR stable enough for you to work off of but it's pretty easy so I'll just do it now

anjor · 2026-02-25T10:54:38Z

service/dealpusher/dealpusher.go

 			}

+			// market deals need the on-chain actor for the deal proposal
+			if walletObj.ActorID == nil {


This hard-fails schedules when wallet.ActorID == nil.

Given import now supports offline wallets (ActorID=nil), this blocks newly imported wallets from ever progressing unless actor linking happens elsewhere. I only see GetOrCreateActor defined, not used.

Can we resolve/link actor here (or earlier in wallet selection/import flow) before returning schedule error?

anjor · 2026-02-25T10:54:39Z

handler/deal/send-manual.go


-	dealModel, err := dealMaker.MakeDeal(ctx, wallet, car, dealConfig)
+	// resolve wallet for signing
+	walletRecord, err := wallet.LoadWalletByActorID(ctx, db, actor.ID)


Manual deal path now requires a pre-linked wallet for the actor (LoadWalletByActorID), so it fails for wallets imported offline and not yet linked.

Should this path also perform lazy actor/wallet linking (similar to GetOrCreateActor) so first use works end-to-end?

anjor · 2026-02-25T10:54:40Z

model/wallet.go

+// private key stored in external keystore, can be linked to on-chain actor
+// wallets can exist before actors are created on-chain
+type Wallet struct {
+	ID uint `gorm:"primaryKey" json:"id"`


model.Wallet now uses a numeric auto-increment PK (ID uint).

Can we also update fixPostgresSequences to include wallets in sequenceTables? Otherwise restored/imported DBs with explicit wallet IDs can end up with stale wallets_id_seq and fail subsequent inserts.

given that we are still supporting multiple backends I would fix this the same way we fix the other sequences (in admin init on every startup)

tbh I would love to drop mysql support, there is just no reason for it, but sqlite for small/test use cases remains useful

EDIT: yes ok on closer reading that's exactly what you're suggesting, yes will do

anjor · 2026-02-25T10:54:41Z

Thanks for the large refactor here, this is moving in the right direction. I reviewed with focus on behavior/regression risk in wallet/actor separation and found 3 blocking issues:

/wallet/{address} delete semantics regressed:
- RemoveHandler now deletes Actor instead of Wallet, but endpoint/docs are still wallet-scoped.
- Result: wallet delete can succeed while wallet row (and key reference) remains.
Imported wallets are not usable on first market-deal path:
- Import creates wallets with ActorID=nil (expected for offline import),
- but deal execution now hard-fails when ActorID is nil,
- and the lazy-link helper (GetOrCreateActor) appears defined but not wired into runtime flow.
Postgres sequence maintenance missed new wallet PK:
- Wallet.ID is now numeric auto-increment,
- but fixPostgresSequences does not include wallets,
- so restored/imported DBs can hit stale wallets_id_seq and fail inserts.

I left inline comments at exact code locations for each item.

parkan · 2026-02-25T11:19:03Z

there are other potential QoL improvements like giving wallets human readable labels etc (some of that was done in the discarded develop branch but with way too much detail) but let's leave that for later

resolve conflicts in dealpusher: adapt PDP scheduling to new Wallet/Actor model and keystore-based EVMSigner from #622

parkan mentioned this pull request Feb 20, 2026

Implement PDP schedule execution path and config wiring #621

Merged

anjor force-pushed the feat/keystore-signer branch from 1a3935e to a494cf5 Compare February 24, 2026 13:58

anjor force-pushed the feat/keystore-signer branch from 449f85d to 1202206 Compare February 24, 2026 14:18

parkan added 10 commits February 24, 2026 17:23

standard keystore implementation

4b2c764

rename old Wallet to Actor, temporary WalletKey to Wallet, remove old…

31b1e67

… import code

complete wallet-agent migration

de6203d

fix actor/wallet references in tests

d8bca70

wire go-synapse signer into keystore, drop go-filsigner

60187f7

fix Wallet→Actor references in shovel and pdp scheduling code

1d8c815

add ImportKeystoreHandler integration tests

cbc5a6c

refactor MakeDeal to accept ProposalSigner instead of db+keystore

af5cdb4

Callers (dealpusher, send-manual handler) now resolve the wallet and build a signing closure before calling MakeDeal, keeping the deal maker free of keystore and database concerns.

harmonize wallet/actor dichotomy

033d760

PDPProofSetManager takes EVMSigner, not model.Actor

ee0f83b

parkan force-pushed the feat/keystore-signer branch from e5d498a to ee0f83b Compare February 24, 2026 16:26

gofmt

21bbd98

parkan marked this pull request as ready for review February 24, 2026 16:37

parkan added 2 commits February 25, 2026 09:17

prepare for easier rebase for 621

8955bb0

fix duplicate key and keystore I/O error handling

2ded76b

parkan changed the title ~~wire go-synapse signer into keystore, drop go-filsigner~~ wire go-synapse signer into keystore (vs go-filsigner), fix Wallet/Actor semantics Feb 25, 2026

anjor reviewed Feb 25, 2026

View reviewed changes

PR feedback

710ebb1

parkan merged commit d8088ee into main Feb 25, 2026
3 checks passed

parkan deleted the feat/keystore-signer branch February 25, 2026 11:48

parkan added a commit that referenced this pull request Feb 25, 2026

merge main (including #622) into feat/pdp-scheduling-runner

c8c4405

resolve conflicts in dealpusher: adapt PDP scheduling to new Wallet/Actor model and keystore-based EVMSigner from #622

Conversation

parkan commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

New keystore abstraction (util/keystore/):

Model rename — old Wallet becomes Actor:

New Wallet model (model/wallet.go):

Preparation associates with Wallet, not Actor:

MakeDeal takes ProposalSigner instead of db + keystore:

Lazy actor resolution (handler/wallet/sign.go):

DealPusher options pattern (service/dealpusher/options.go):

Dependency changes

What's NOT in this PR

Uh oh!

parkan commented Feb 24, 2026

Uh oh!

parkan commented Feb 24, 2026

Uh oh!

parkan commented Feb 24, 2026

Uh oh!

parkan commented Feb 25, 2026

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!

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!

anjor commented Feb 25, 2026

Uh oh!

parkan commented Feb 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parkan commented Feb 20, 2026 •

edited

Loading