Single Process Piri Node by frrist · Pull Request #192 · storacha/piri

frrist · 2025-08-13T00:01:13Z

Dependencies & Review

This PR builds on #190
I would recommend reviewing each commit of this PR on it's own - I have done my best to keep them well scoped.

What

This PR makes life easier by combining two servers into one. Instead of juggling piri serve ucan and piri server pdp in separate processes, you can now just run piri serve full and call it a day.

The new command runs both UCAN and PDP servers in a single process. Don't worry if you're using an external PDP implementation (like Curio) - piri serve ucan still works standalone.

Here's what the config looks like - notice how both UCAN and PDP settings live together:

[identity]
key_file = '/etc/piri/service.pem'

[pdp]
contract_address = '0x6170dE2b09b404776197485F3dc6c968Ef948505'
lotus_endpoint = 'wss://lotus.spicystorage.tech/rpc/v1'
owner_address = '0x7469B47e006D0660aB92AE560b27A1075EEcF97F'

[repo]
data_dir = '/data/piri'
temp_dir = '/tmp/storage'

[server]
host = 'localhost'
port = '3000'
public_url = 'https://piri.spicystorage.tech'

[ucan]
proof_set = '414'

[ucan.services]
[ucan.services.indexer]
did = 'did:web:staging.indexer.warm.storacha.network'
url = 'https://staging.indexer.warm.storacha.network/claims'
proof = '<proof>'

[ucan.services.principal_mapping]
'did:web:staging.indexer.warm.storacha.network' = 'did:key:z6Mkr4QkdinnXQmJ9JdnzwhcEjR8nMnuVPEwREyh9jp2Pb7k'
'did:web:staging.up.warm.storacha.network' = 'did:key:z6MkpR58oZpK7L3cdZZciKT25ynGro7RZm6boFouWQ7AzF7v'

[ucan.services.publisher]
ipni_announce_urls = ['https://cid.contact/announce', 'https://staging.ipni.warm.storacha.network']

[ucan.services.upload]
did = 'did:web:staging.up.warm.storacha.network'
url = 'https://staging.up.warm.storacha.network'

How

This brings together work from several previous PRs (#187, #186, #169, #161, #144) to let both UCAN and PDP modules run in the same Piri process. Here's what changed under the hood:

Architecture Changes:

Refactored the config system to be composable - configs are now split into logical sections (identity, server, repo, ucan, pdp) that can be mixed and matched
Introduced a provider pattern for core components (wallet, stores, engines, aggregators) so they can be shared between modules
Created separate fx modules (UCANModule and PDPModule) that encapsulate their respective dependencies

The Magic:

The new piri serve full command uses fx (Uber's DI framework) to inject both modules at startup
Both modules share common dependencies through CommonModules (identity, HTTP server, databases)
Each module brings its own routes, services, and background tasks - they play nice together in the same process

Backwards Compatibility:

piri serve ucan still works for operators who want to run just the UCAN server
The old piri server pdp command is gone (replaced by the full command)
All existing config options and environment variables are preserved

hannahhoward

Left various comments, none blocking. Overall, really really nicely done. I have only minor quibbles.

Not marking any of my changes blocking since I'm headed out. But honestly, I would like to see the swear words removed. Sorry for being that bitch. :)

hannahhoward · 2025-08-15T19:44:01Z

 	aggregateSubmitterQueue := aws.NewSQSAggregateQueue(cfg.Config, cfg.SQSPDPPieceAggregatorURL)
-	aggregateSubmitter := aggregator.NewAggregateSubmitteer(cfg.PDPProofSet, aggregateStore, apiClient, aggregateSubmitterQueue)
+	aggregateSubmitter := aggregator.NewAggregateSubmitteer(
+		&aggregator.ConfiguredProofSetProvider{ID: cfg.PDPProofSet},


why not just pass the ID direct?

I had ideas around a different implementation of ProofSetProvider (ConfiguredProofSetProvider implements this interface) which lazily creates a proof set if one doesn't exist when it comes time to submit aggregates.

hannahhoward · 2025-08-15T20:07:04Z

 	}
 }
+
+func initLogging() {


hannahhoward · 2025-08-15T20:13:32Z

 	switch {
 	case err != nil && g.level >= logger.Error:
-		// Always log SQL errors with call stack
+		// Skip logging "record not found" errors for pdp_piece_mh_to_commp table


unrelated change correct?

Is it not useful altogether? Why not relegate to warning?

The pdp_piece_mh_to_commp table is a mapping of (mostly) sha256 multihash to commp multihash. It's the table we frequently use to determine if a piece has been stored with us before. Therefore, we query this table a lot for existence checks, and it frequently doesn't find the cid when there is a new allocation - as expected. Returning an error or a warning here isn't appropriate since there isn't technically an error or a warning that the user needs to be made aware of - it's an in-actionable log message.

This log became spamy during my local testing/dev here, so I modified it to be less spamy.

hannahhoward · 2025-08-15T20:16:43Z


 	// Create SQLite database connection
-	db, err := sqlitedb.New(cfg.Replicator.DBPath)
+	db, err := sqlitedb.New(cfg.Replicator.DBPath,


again related to change or no?

related to single process I mean

The new options? Mostly yes.

hannahhoward · 2025-08-15T20:19:36Z

+	return s.aggregator
+}
+
+func ProvidePoorlyNamedPDPInterface(service types.API, agg aggregator.Aggregator, cfg app.AppConfig) (*Shit, error) {


OMG. man this is painful though I get it. look I don't want to make a big deal but sir this is a wendys. can we not use the actual S-word in the code? Just in case we forget to change it?

+1 for no swears

🙈 sorry!!!! I'll fix this, didn't mean to push it.

hannahhoward · 2025-08-15T20:26:19Z

-func (p *PDPService) FindPiece(ctx context.Context, piece types.Piece) (cid.Cid, bool, error) {
+func (p *PDPService) FindPiece(ctx context.Context, piece types.Piece) (_ cid.Cid, _ bool, retErr error) {
+	log.Infow("finding piece", "request", piece)
+	defer func() {


same viewpoint as above

Same here, retErr is not assigned to...I assume this is what @hannahhoward is also referring to? Maybe I don't know Go good enough...?

same same but different: https://github.com/storacha/piri/pull/192/files#r2285712328

hannahhoward · 2025-08-15T20:26:22Z

+	start := time.Now()
+	log.Infow("uploading piece", "request", pieceUpload)
+	defer func() {
+		if retErr != nil {


I guess this will be a thing then :P

Question: all these logs are unrelated to the single process refactor yes? generally I would put those in a seperate PR. Not a big deal though.

hannahhoward · 2025-08-15T20:27:39Z

+)
+
+// MarshalLogObject implements zapcore.ObjectMarshaler for ProofSetStatus
+func (p ProofSetStatus) MarshalLogObject(enc zapcore.ObjectEncoder) error {


again nice refactor! again for the future I would leave this sort of thing out of an already large PR. but nb

Yes sorry, did this for debugging. Will try to avoid things like this on big changes in the future.

alanshaw

I don't think I have any hard blocks here, but I do think it would be good to move the logging changes to a seperate PR since they're unrelated (also because I'm not sure they will actually do any logging).

I would really like piri serve full to just be piri serve though..."full" just screams legacy baggage IMHO.

alanshaw · 2025-08-19T13:02:28Z

+		logging.SetLogLevel("auth", "error")
+		logging.SetLogLevel("rpcenc", "error")
+		logging.SetLogLevel("storage", "info")
+		logging.SetLogLevel("resources", "error")


The default is error right? Why not expicitly set all loggers to error and then just change the ones you want to change?

ohh, yeah good point that would be a simpler change and easier to reason about - will address.

alanshaw · 2025-08-19T13:06:24Z

+		Use:   "full",
+		Short: "Start the full piri server!",
+		Args:  cobra.NoArgs,
+		RunE:  fullServer,


Can it just be piri serve?

While I don't disagree - what becomes of piri serve ucan? Perhaps enabling the PDP mode ought to be a configuration value or flag on the serve command itself?

or the other way around, as in piri serve --ucan-only

Are we okay with merging this as is, and addressing this comment(s) in a follow on?

alanshaw · 2025-08-19T13:08:17Z

+
+	FullCmd.Flags().String(
+		"contract-address",
+		"0x6170dE2b09b404776197485F3dc6c968Ef948505", // NB(forrest): default to calibration contract addrese


Do you think we should move this to presets?

humm, yeah that would probably be a better place for this, will address.

alanshaw · 2025-08-19T13:14:37Z

 	switch {
 	case err != nil && g.level >= logger.Error:
-		// Always log SQL errors with call stack
+		// Skip logging "record not found" errors for pdp_piece_mh_to_commp table


Is it not useful altogether? Why not relegate to warning?

alanshaw · 2025-08-19T13:15:00Z

+			fx.As(new(aggregator.Aggregator)),
+		),
+		aggregator.NewPieceAccepter,
+		aggregator.NewAggregateSubmitteer,


Suggested change

aggregator.NewAggregateSubmitteer,

aggregator.NewAggregateSubmitter,

Submitteer predates my changes, and I had kinda just assumed Submitteer was the british-english spelling. Will change!

alanshaw · 2025-08-19T13:37:16Z


 type PieceAccepter struct {
-	issuer         ucan.Signer
+	issuer         principal.Signer


Why the switch here and other places?

Because the identity is provided and injected as a principal.Signer here: https://github.com/storacha/piri/blob/main/pkg/fx/identity/provider.go#L15
we could modify this fx code I linked to such that:

var Module = fx.Module("identity", fx.Provide( ProvideIdentity, fx.Annotate( fx.As(fx.Self()), fx.As(new(ucan.Signer)), ), ), )

which will provide the identity as both interfaces - ucan.Signer and principal.Signer, allowing other methods to depend on either interface, but sticking with one here - principal.Signer because that feels simpler - fewer interfaces.

alanshaw · 2025-08-19T13:38:24Z

+		LogStatus:        true,
+		LogContentLength: true,
+		LogResponseSize:  true,
+		LogHeaders:       []string{"X-Agent-Message"},


😮 wow are you skating to where the puck will be? ❤️

⛸️ 🕺 Yup! Trying to align with storacha/go-ucanto#48 here

alanshaw · 2025-08-19T13:41:57Z

+		} else {
+			log.Infow("allocated piece", "request", allocation, "response", res)
+		}
+	}()


Does this work? I don't think the function assigns to retErr anywhere?

Yes because of the change in the function signature using named result parameters:
(res *types.AllocatedPiece, retErr error)
retErr will contain an error if this function returns an error, same for res containing *types.AllocatedPiece if the function executes successfully.

alanshaw · 2025-08-19T13:42:29Z

-func (p *PDPService) FindPiece(ctx context.Context, piece types.Piece) (cid.Cid, bool, error) {
+func (p *PDPService) FindPiece(ctx context.Context, piece types.Piece) (_ cid.Cid, _ bool, retErr error) {
+	log.Infow("finding piece", "request", piece)
+	defer func() {


Same here, retErr is not assigned to...I assume this is what @hannahhoward is also referring to? Maybe I don't know Go good enough...?

alanshaw · 2025-08-19T13:44:55Z

+	log.Infow("uploading piece", "request", pieceUpload)
+	defer func() {
+		if retErr != nil {
+			log.Errorw("failed to upload piece", "request", pieceUpload, "duration", time.Since(start), "error", retErr)


Is the duration useful in the log? Can it be added to telemetry instead? Maybe this is a seperate PR?

Yes these should be metrics emitted from the node. I added in these logs while verifying that all APIs and their corresponding services were registered w/ fx. Should have been a separate change, but landed in here for my own convenience.

volmedo · 2025-08-19T12:45:38Z

-			func(svc *claims.ClaimService) claims.Claims {
-				return svc
-			},
+			NewService,


can we now get rid of the NewService -> claims.NewV2 thing now?

I'd like to do that in a follow on, where we also move a bunch of the fx provider codes closer to the types they are providing. Also, NewService is still used for the AWS setup iirc.

volmedo · 2025-08-19T12:47:46Z

-			func(svc *publisher.PublisherService) publisher.Publisher {
-				return svc
-			},
+			NewService,


same here, I'd expect the logic in NewService to be part of publisher.New in the service/publisher package

Fully agree, mentioned this here: https://github.com/storacha/piri/pull/192/files#r2285609301 would prefer to wait and do this in a follow on.

volmedo · 2025-08-19T12:52:44Z

+var Module = fx.Module("root-handler",
+	fx.Provide(
+		fx.Annotate(
+			NewRootHandler,


maybe move this to the pkg/server package?

Agreed, follow item: https://github.com/storacha/piri/pull/192/files#r2285609301

volmedo · 2025-08-19T13:00:23Z


 	"github.com/ipld/go-ipld-prime/datamodel"
-	"github.com/storacha/go-libstoracha/capabilities/types"
+	libtypes "github.com/storacha/go-libstoracha/capabilities/types"


this is usually aliased as captypes in other repos (just a convention)

volmedo · 2025-08-19T13:00:36Z

 	"github.com/storacha/piri/pkg/pdp/aggregator/aggregate"
 	"github.com/storacha/piri/pkg/pdp/aggregator/fns"
-	types2 "github.com/storacha/piri/pkg/pdp/types"
+	types "github.com/storacha/piri/pkg/pdp/types"


this is redundant now

Suggested change

types "github.com/storacha/piri/pkg/pdp/types"

"github.com/storacha/piri/pkg/pdp/types"

volmedo · 2025-08-19T13:08:39Z

+	return db, nil
+}
+
+func ProviderAggregatorDB(lc fx.Lifecycle, cfg app.StorageConfig) (*sql.DB, error) {


Suggested change

func ProviderAggregatorDB(lc fx.Lifecycle, cfg app.StorageConfig) (*sql.DB, error) {

func ProvideAggregatorDB(lc fx.Lifecycle, cfg app.StorageConfig) (*sql.DB, error) {

volmedo · 2025-08-19T13:53:36Z

 	}
 	blobStore := blobstore.NewTODO_DsBlobstore(namespace.Wrap(ds, datastore.NewKey("blobs")))
-	stashStore, err := store.NewStashStore(path.Join(dataDir))
+	stashStore, err := stashstore.NewStashStore(path.Join(dataDir))


now that the store package is renamed to stashstore, stashstore.NewStashStore could be renamed to stashstore.New

Agreed, saving this for a follow on where we do more mechanical cleanups.

volmedo · 2025-08-19T13:57:56Z

+		Use:   "full",
+		Short: "Start the full piri server!",
+		Args:  cobra.NoArgs,
+		RunE:  fullServer,


volmedo · 2025-08-19T14:01:42Z

+// AsRouteRegistrar provides the Handler as a RouteRegistrar
+func AsRouteRegistrar(h *Handler) echofx.RouteRegistrar {
+	return h
+}
+


I thought this was what fx.As was for?

required for full node when proof set isn't know at aggregator startup time. i.e allows lazy fetching and creating of a proofset.

frrist requested a review from alanshaw as a code owner August 13, 2025 00:01

This was referenced Aug 13, 2025

Frrist/feat/one server #191

Closed

Frrist/single process #175

Closed

frrist self-assigned this Aug 13, 2025

frrist force-pushed the frrist/one-piri branch from bfacaac to c24b031 Compare August 13, 2025 20:20

frrist requested review from hannahhoward and volmedo August 14, 2025 00:30

frrist force-pushed the frrist/one-piri branch 6 times, most recently from 66fa621 to bcef559 Compare August 14, 2025 17:24

frrist changed the title ~~Frrist/one piri~~ Single Process Piri Node Aug 14, 2025

hannahhoward approved these changes Aug 15, 2025

View reviewed changes

alanshaw approved these changes Aug 19, 2025

View reviewed changes

volmedo approved these changes Aug 19, 2025

View reviewed changes

frrist force-pushed the frrist/one-piri branch from bcef559 to bc0063a Compare August 19, 2025 21:57

frrist changed the base branch from frrist/refactor/simplify-config to main August 19, 2025 21:58

frrist added 8 commits August 19, 2025 14:58

fix: correctly register routes for ucan server

6e02109

feat: add proof set provider to aggregate submitted

95b15f7

required for full node when proof set isn't know at aggregator startup time. i.e allows lazy fetching and creating of a proofset.

feat: create aggregator fx module

57b0fa5

feat: create aggregator and task engine db provider

e398701

feat: add engine provider

62e1ed5

feat: add store providers, move stashstore to store pkg

748f133

feat: add wallet provider

1acfb66

feat: implement single process operation

3df6a33

frrist force-pushed the frrist/one-piri branch from bc0063a to 3df6a33 Compare August 19, 2025 21:59

frrist added 2 commits August 19, 2025 15:34

address feedback

7684d14

fix delegate command config loading

5a33e5d

frrist force-pushed the frrist/one-piri branch from 98916c2 to fbd91a6 Compare August 20, 2025 20:21

fix panics and wallet command

2288012

frrist force-pushed the frrist/one-piri branch from fbd91a6 to 2288012 Compare August 20, 2025 20:25

frrist mentioned this pull request Aug 21, 2025

fix: explicitly log handler execution errors #196

Merged

frrist merged commit 021feb4 into main Aug 21, 2025
8 checks passed

frrist mentioned this pull request Aug 21, 2025

Update docs for single process piri operation #198

Closed

frrist added this to the v0.0.13 release milestone Aug 26, 2025

frrist mentioned this pull request Aug 29, 2025

Unify Piri UCAN Server and PDP Server as single process #116

Closed

	aggregator.NewAggregateSubmitteer,
	aggregator.NewAggregateSubmitter,

	types "github.com/storacha/piri/pkg/pdp/types"
	"github.com/storacha/piri/pkg/pdp/types"

	func ProviderAggregatorDB(lc fx.Lifecycle, cfg app.StorageConfig) (*sql.DB, error) {
	func ProvideAggregatorDB(lc fx.Lifecycle, cfg app.StorageConfig) (*sql.DB, error) {

Conversation

frrist commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependencies & Review

What

How

Uh oh!

hannahhoward 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!

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!

alanshaw Aug 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

alanshaw 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!

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

frrist commented Aug 13, 2025 •

edited

Loading

alanshaw Aug 19, 2025 •

edited

Loading