From 40a910a22244059f8d6428032f065e137fc970db Mon Sep 17 00:00:00 2001 From: Dirk McCormick Date: Mon, 31 May 2021 08:45:05 -0600 Subject: [PATCH 1/2] fix: for retrievals dont round down payments to lower interval bound --- .../impl/clientstates/client_states.go | 22 +++++++++---------- retrievalmarket/impl/dtutils/dtutils.go | 4 ++-- retrievalmarket/impl/integration_test.go | 12 +++++----- .../storage_retrieval_integration_test.go | 8 +++---- storagemarket/impl/dtutils/dtutils.go | 4 ++-- 5 files changed, 24 insertions(+), 26 deletions(-) diff --git a/retrievalmarket/impl/clientstates/client_states.go b/retrievalmarket/impl/clientstates/client_states.go index fe7918d5..bdfb515c 100644 --- a/retrievalmarket/impl/clientstates/client_states.go +++ b/retrievalmarket/impl/clientstates/client_states.go @@ -133,18 +133,16 @@ func ProcessPaymentRequested(ctx fsm.Context, environment ClientDealEnvironment, func SendFunds(ctx fsm.Context, environment ClientDealEnvironment, deal rm.ClientDealState) error { totalBytesToPayFor := deal.TotalReceived - // If unsealing has been paid for, and not all blocks have been received - if deal.UnsealFundsPaid.GreaterThanEqual(deal.UnsealPrice) && !deal.AllBlocksReceived { - // If the number of bytes received is less than the number required - // for the current payment interval, no need to send a payment - if totalBytesToPayFor < deal.CurrentInterval { - log.Debugf("client: ignoring payment request for %d: total bytes to pay for %d < interval %d", - deal.PaymentRequested, totalBytesToPayFor, deal.CurrentInterval) - return ctx.Trigger(rm.ClientEventPaymentNotSent) - } - - // Otherwise round the number of bytes to pay for down to the current interval - totalBytesToPayFor = deal.CurrentInterval + // If unsealing has been paid for, and not all blocks have been received, + // and the number of bytes received is less than the number required + // for the current payment interval, no need to send a payment + if deal.UnsealFundsPaid.GreaterThanEqual(deal.UnsealPrice) && + !deal.AllBlocksReceived && + totalBytesToPayFor < deal.CurrentInterval { + + log.Debugf("client: ignoring payment request for %d: total bytes to pay for %d < interval %d", + deal.PaymentRequested, totalBytesToPayFor, deal.CurrentInterval) + return ctx.Trigger(rm.ClientEventPaymentNotSent) } tok, _, err := environment.Node().GetChainHead(ctx.Context()) diff --git a/retrievalmarket/impl/dtutils/dtutils.go b/retrievalmarket/impl/dtutils/dtutils.go index ce98ef6f..f854c798 100644 --- a/retrievalmarket/impl/dtutils/dtutils.go +++ b/retrievalmarket/impl/dtutils/dtutils.go @@ -179,7 +179,7 @@ func TransportConfigurer(thisPeer peer.ID, storeGetter StoreGetter) datatransfer otherPeer := channelID.OtherParty(thisPeer) store, err := storeGetter.Get(otherPeer, dealProposal.ID) if err != nil { - log.Errorf("attempting to configure data store: %w", err) + log.Errorf("attempting to configure data store: %s", err) return } if store == nil { @@ -187,7 +187,7 @@ func TransportConfigurer(thisPeer peer.ID, storeGetter StoreGetter) datatransfer } err = gsTransport.UseStore(channelID, store.Loader, store.Storer) if err != nil { - log.Errorf("attempting to configure data store: %w", err) + log.Errorf("attempting to configure data store: %s", err) } } } diff --git a/retrievalmarket/impl/integration_test.go b/retrievalmarket/impl/integration_test.go index 6e9b15e6..50e47b23 100644 --- a/retrievalmarket/impl/integration_test.go +++ b/retrievalmarket/impl/integration_test.go @@ -259,7 +259,7 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { {name: "multi-block file retrieval succeeds", filename: "lorem.txt", filesize: 19000, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, }, {name: "multi-block file retrieval with zero price per byte succeeds", filename: "lorem.txt", @@ -269,7 +269,7 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { {name: "multi-block file retrieval succeeds with V1 params and AllSelector", filename: "lorem.txt", filesize: 19000, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, paramsV1: true, selector: shared.AllSelector()}, {name: "partial file retrieval succeeds with V1 params and selector recursion depth 1", @@ -290,7 +290,7 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { {name: "succeeds for regular blockstore", filename: "lorem.txt", filesize: 19000, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, skipStores: true, }, { @@ -303,14 +303,14 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { {name: "multi-block file retrieval succeeds, final block exceeds payment interval", filename: "lorem.txt", filesize: 19000, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(9000000), abi.NewTokenAmount(19250000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(9112000), abi.NewTokenAmount(19352000), abi.NewTokenAmount(19920000)}, paymentInterval: 9000, paymentIntervalIncrease: 1250, }, {name: "multi-block file retrieval succeeds, final block lands on payment interval", filename: "lorem.txt", filesize: 19000, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(9000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(9112000), abi.NewTokenAmount(19920000)}, // Total bytes: 19,920 // intervals: 9,000 | 9,000 + (9,000 + 1920) paymentInterval: 9000, @@ -320,7 +320,7 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { filename: "lorem.txt", filesize: 19000, disableNewDeals: true, - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, }, } diff --git a/retrievalmarket/storage_retrieval_integration_test.go b/retrievalmarket/storage_retrieval_integration_test.go index e9300068..48cea997 100644 --- a/retrievalmarket/storage_retrieval_integration_test.go +++ b/retrievalmarket/storage_retrieval_integration_test.go @@ -61,7 +61,7 @@ func TestStorageRetrieval(t *testing.T) { pricePerByte: abi.NewTokenAmount(1000), paymentInterval: uint64(10000), paymentIntervalIncrease: uint64(1000), - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, }, "zero unseal, zero price per byte": { @@ -77,7 +77,7 @@ func TestStorageRetrieval(t *testing.T) { pricePerByte: abi.NewTokenAmount(1000), paymentInterval: uint64(10000), paymentIntervalIncrease: uint64(1000), - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(1000), abi.NewTokenAmount(10001000), abi.NewTokenAmount(19921000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(1000), abi.NewTokenAmount(10137000), abi.NewTokenAmount(19921000)}, }, } @@ -124,7 +124,7 @@ func TestOfflineStorageRetrieval(t *testing.T) { pricePerByte: abi.NewTokenAmount(1000), paymentInterval: uint64(10000), paymentIntervalIncrease: uint64(1000), - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10000000), abi.NewTokenAmount(19920000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, }, "zero unseal, zero price per byte": { @@ -140,7 +140,7 @@ func TestOfflineStorageRetrieval(t *testing.T) { pricePerByte: abi.NewTokenAmount(1000), paymentInterval: uint64(10000), paymentIntervalIncrease: uint64(1000), - voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(1000), abi.NewTokenAmount(10001000), abi.NewTokenAmount(19921000)}, + voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(1000), abi.NewTokenAmount(10137000), abi.NewTokenAmount(19921000)}, }, } diff --git a/storagemarket/impl/dtutils/dtutils.go b/storagemarket/impl/dtutils/dtutils.go index 66f6e841..e7f4bda2 100644 --- a/storagemarket/impl/dtutils/dtutils.go +++ b/storagemarket/impl/dtutils/dtutils.go @@ -135,7 +135,7 @@ func TransportConfigurer(storeGetter StoreGetter) datatransfer.TransportConfigur } store, err := storeGetter.Get(storageVoucher.Proposal) if err != nil { - log.Errorf("attempting to configure data store: %w", err) + log.Errorf("attempting to configure data store: %s", err) return } if store == nil { @@ -143,7 +143,7 @@ func TransportConfigurer(storeGetter StoreGetter) datatransfer.TransportConfigur } err = gsTransport.UseStore(channelID, store.Loader, store.Storer) if err != nil { - log.Errorf("attempting to configure data store: %w", err) + log.Errorf("attempting to configure data store: %s", err) } } } From 6a94d16ad18fd9ae18f0725500a923fa66419a0d Mon Sep 17 00:00:00 2001 From: Dirk McCormick Date: Tue, 1 Jun 2021 10:10:58 -0600 Subject: [PATCH 2/2] feat: update to go-data-transfer v1.6.0 --- go.mod | 2 +- go.sum | 4 ++-- retrievalmarket/retrieval_restart_integration_test.go | 2 -- storagemarket/integration_test.go | 3 +-- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index e6d64256..91714f35 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/filecoin-project/go-address v0.0.3 github.com/filecoin-project/go-cbor-util v0.0.0-20191219014500-08c40a1e63a2 github.com/filecoin-project/go-commp-utils v0.0.0-20201119054358-b88f7a96a434 - github.com/filecoin-project/go-data-transfer v1.5.0 + github.com/filecoin-project/go-data-transfer v1.6.0 github.com/filecoin-project/go-ds-versioning v0.1.0 github.com/filecoin-project/go-multistore v0.0.3 github.com/filecoin-project/go-padreader v0.0.0-20200903213702-ed5fae088b20 diff --git a/go.sum b/go.sum index 0474f3c8..8d589fcb 100644 --- a/go.sum +++ b/go.sum @@ -109,8 +109,8 @@ github.com/filecoin-project/go-commp-utils v0.0.0-20201119054358-b88f7a96a434/go github.com/filecoin-project/go-crypto v0.0.0-20191218222705-effae4ea9f03 h1:2pMXdBnCiXjfCYx/hLqFxccPoqsSveQFxVLvNxy9bus= github.com/filecoin-project/go-crypto v0.0.0-20191218222705-effae4ea9f03/go.mod h1:+viYnvGtUTgJRdy6oaeF4MTFKAfatX071MPDPBL11EQ= github.com/filecoin-project/go-data-transfer v1.0.1/go.mod h1:UxvfUAY9v3ub0a21BSK9u3pB2aq30Y0KMsG+w9/ysyo= -github.com/filecoin-project/go-data-transfer v1.5.0 h1:eXmcq7boRl/S3plV0/h4qdxkM6EgFIXF9y3UdOL0VXE= -github.com/filecoin-project/go-data-transfer v1.5.0/go.mod h1:E3WW4mCEYwU2y65swPEajSZoFWFmfXt7uwGduoACZQc= +github.com/filecoin-project/go-data-transfer v1.6.0 h1:DHIzEc23ydRCCBwtFet3MfgO8gMpZEnw60Y+s71oX6o= +github.com/filecoin-project/go-data-transfer v1.6.0/go.mod h1:E3WW4mCEYwU2y65swPEajSZoFWFmfXt7uwGduoACZQc= github.com/filecoin-project/go-ds-versioning v0.1.0 h1:y/X6UksYTsK8TLCI7rttCKEvl8btmWxyFMEeeWGUxIQ= github.com/filecoin-project/go-ds-versioning v0.1.0/go.mod h1:mp16rb4i2QPmxBnmanUx8i/XANp+PFCCJWiAb+VW4/s= github.com/filecoin-project/go-fil-commcid v0.0.0-20200716160307-8f644712406f/go.mod h1:Eaox7Hvus1JgPrL5+M3+h7aSPHc0cVqpSxA+TxIEpZQ= diff --git a/retrievalmarket/retrieval_restart_integration_test.go b/retrievalmarket/retrieval_restart_integration_test.go index df26f34a..5b4b72dc 100644 --- a/retrievalmarket/retrieval_restart_integration_test.go +++ b/retrievalmarket/retrieval_restart_integration_test.go @@ -88,7 +88,6 @@ func TestBounceConnectionDealTransferOngoing(t *testing.T) { restartConf := dtimpl.ChannelRestartConfig(channelmonitor.Config{ AcceptTimeout: 100 * time.Millisecond, RestartBackoff: 100 * time.Millisecond, - RestartAckTimeout: 2 * time.Second, RestartDebounce: 100 * time.Millisecond, MaxConsecutiveRestarts: 5, CompleteTimeout: 100 * time.Millisecond, @@ -212,7 +211,6 @@ func TestBounceConnectionDealTransferUnsealing(t *testing.T) { restartConf := dtimpl.ChannelRestartConfig(channelmonitor.Config{ AcceptTimeout: 100 * time.Millisecond, RestartBackoff: 100 * time.Millisecond, - RestartAckTimeout: 2 * time.Second, RestartDebounce: 100 * time.Millisecond, MaxConsecutiveRestarts: 5, CompleteTimeout: 100 * time.Millisecond, diff --git a/storagemarket/integration_test.go b/storagemarket/integration_test.go index 49a5da05..f33c6f0e 100644 --- a/storagemarket/integration_test.go +++ b/storagemarket/integration_test.go @@ -179,6 +179,7 @@ func TestMakeDealOffline(t *testing.T) { ctx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() h := testharness.NewHarness(t, ctx, true, noOpDelay, noOpDelay, false) + shared_testutil.StartAndWaitForReady(ctx, t, h.Provider) shared_testutil.StartAndWaitForReady(ctx, t, h.Client) @@ -301,7 +302,6 @@ func TestRestartOnlyProviderDataTransfer(t *testing.T) { restartConf := dtimpl.ChannelRestartConfig(channelmonitor.Config{ AcceptTimeout: 100 * time.Millisecond, RestartBackoff: 100 * time.Millisecond, - RestartAckTimeout: 2 * time.Second, RestartDebounce: 100 * time.Millisecond, MaxConsecutiveRestarts: 5, CompleteTimeout: 100 * time.Millisecond, @@ -637,7 +637,6 @@ func TestBounceConnectionDataTransfer(t *testing.T) { restartConf := dtimpl.ChannelRestartConfig(channelmonitor.Config{ AcceptTimeout: 100 * time.Millisecond, RestartBackoff: 100 * time.Millisecond, - RestartAckTimeout: 2 * time.Second, RestartDebounce: 100 * time.Millisecond, MaxConsecutiveRestarts: 5, CompleteTimeout: 100 * time.Millisecond,