migrate provider deal proposal label to the new format #721
Conversation
dirkmc
force-pushed
the
fix/migrate-dealprop-label
branch
3 times, most recently
from
d5d8db3
to
308c548
Compare
dirkmc
force-pushed
the
fix/migrate-dealprop-label
branch
from
308c548
to
4db0b66
Compare
dirkmc
force-pushed
the
fix/migrate-dealprop-label
branch
3 times, most recently
from
db2a6da
to
72804de
Compare
dirkmc
force-pushed
the
fix/migrate-dealprop-label
branch
from
72804de
to
fd8afb8
Compare
go.mod
Outdated
| @@ -3,6 +3,7 @@ module github.com/filecoin-project/go-fil-markets | |||
| go 1.13 | |||
|
|
|||
| require ( | |||
| github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1 // indirect | |||
There was a problem hiding this comment.
This is such an odd dep... curious what did pull it in?
There was a problem hiding this comment.
This breaks boost build for me; Removing this like fixes it
github.com/filecoin-project/boost/cli/node imports
github.com/libp2p/go-libp2p-core/crypto imports
github.com/btcsuite/btcd/btcec tested by
github.com/btcsuite/btcd/btcec.test imports
github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
github.com/btcsuite/btcd v0.22.0-beta (/home/magik6k/.opt/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1 (/home/magik6k/.opt/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.1)
There was a problem hiding this comment.
This is how the libp2p team fixed it:
libp2p/go-libp2p-core#254
There was a problem hiding this comment.
Working on a fix here:
#723
There was a problem hiding this comment.
Merged the above PR with a fix into this branch.
| prop, err := migrations.MigrateClientDealProposal0To1(*ds.DealProposal) | ||
| if err != nil { | ||
| err = fmt.Errorf("migrating v110 deal proposal to current version: %w", err) | ||
| log.Warnf(err.Error()) | ||
| return ProposalUndefined, cid.Undef, err | ||
| } |
There was a problem hiding this comment.
Is this from the user? We can't do this migration without (possibly) changing signatures.
There was a problem hiding this comment.
Good catch 👍
One option would be to reject non-utf8 labels sent over version 1.1.0 of the deal proposal protocol.
There was a problem hiding this comment.
@dirkmc just for my sanity, this will only affect any new deals made, not historical deals that may have non-utf8 labels, yes?
There was a problem hiding this comment.
Historical deals will be migrated from the old format (deal label as a string) to the new format (DealLabel struct).
If those deals were made with a non-utf8 deal label, their signature will no longer match the new format. It's my understanding that this doesn't matter because those deals have already been published on chain.
This PR introduces a new protocol version. So we now have:
- v1.1.0: The current version
When a deal proposal is received, reject the deal proposal if it has a label that cannot be parsed as utf8.
Otherwise, migrate the deal proposal to the new format (label as DealLabel struct). This should not change the signature of the deal because the label is CBOR-marshalled in the same was as if it were a string. See the tests in storagemarket/network/deal_stream_v110_test.go - v1.1.1: The new version
This protocol version will be used when both the client and provider use a version of lotus that depends on this PR.
Deals made over v1.1.1 are already in the new format (label as a DealLabel struct).
There was a problem hiding this comment.
@dirkmc Got it, I think this is good for any newly made / received deals. My concern is whether the entire migration of historical deals will fail if there are historical non-utf8 deals (which I agree will no longer have valid signatures) -- and if so, what is the impact of this migration failing?
(also when does the migration of historical deal info happen? at startup?)
There was a problem hiding this comment.
The migration happens at startup 👍
The migration is not expected to fail for historical deals with non-utf8 labels. Please take a look at the code here:
https://github.com/filecoin-project/go-fil-markets/pull/721/files#diff-8536cd1664598e5c06e27807ae8fa95a9f1d40439d4883fbef351e54826dbeccR248
If there's an issue with that code let me know.
There was a problem hiding this comment.
Perfect, thanks, that assuages my concern -- I think I was conflating the ReadDealProposal (which uses checkDealLabel and fails if non-utf-8) with the migration code, that succeeds in an immaterially wrong state.
Thanks for indulging!
There was a problem hiding this comment.
Ah yep, that makes sense 👍
|
(this needs to be backported on top of |
dirkmc
force-pushed
the
fix/migrate-dealprop-label
branch
from
960b6ee
to
5132179
Compare
| } | ||
|
|
||
| return &storagemarket.ClientDealProposal{ | ||
| ClientSignature: prop.ClientSignature, |
There was a problem hiding this comment.
Yeah, this isn't entirely meaningful, because if we are using the byte-type label (that is, if the label wasn't a valid UTF-8 string), then the signature is now invalid. I'm not too concerned about getting this right, though...
There was a problem hiding this comment.
Right, I think it's ok for the signature for deals with non-utf8 labels not to match, as long as the deals have already been published on chain
| prop, err := migrations.MigrateClientDealProposal0To1(*ds.DealProposal) | ||
| if err != nil { | ||
| err = fmt.Errorf("migrating v110 deal proposal to current version: %w", err) | ||
| log.Warnf(err.Error()) | ||
| return ProposalUndefined, cid.Undef, err | ||
| } |
There was a problem hiding this comment.
@dirkmc just for my sanity, this will only affect any new deals made, not historical deals that may have non-utf8 labels, yes?
I made a build of v1.16.0-rc1 with go-fil-markets updated to point at this PR.
I tested that I could make a storage deal and it published successfully on my local devnet with the following combinations of client and provider: