batchstore: reserve #1262

zelig · 2021-02-16T10:00:25Z

This PR implements the reserve logic for the batchstore.
it is soon to described in AP https://hackmd.io/9ejVZEvSQg69zXG91YsFEQ
complete together with localstore reserve logic

acud · 2021-02-23T08:58:28Z

is this ready for review?

pkg/postage/batchservice/batchservice_test.go

pkg/postage/batchstore/export_test.go

acud · 2021-02-23T09:24:15Z

pkg/postage/batchstore/export_test.go

+	})
+}
+
+//


you can expose a method on store just for the sake for the tests:
func (s *store) GetReserve() (*bit.Int, uint8) { return s.rs.Core, s.rs.Depth}

but i cannot use type *store in the test if i go proper package batchstore_test can only use type postage.Storer, so I need to define these this way.

https://github.com/ethersphere/bee/blob/master/pkg/retrieval/export_test.go#L13

pkg/postage/batchstore/reserve.go

pkg/postage/batchstore/reserve_test.go

pkg/postage/batchstore/store.go

pkg/storage/mock/batchstore/batchstore.go

pkg/postage/batchstore/reserve_test.go

ralph-pichler

code looks good for the most part. don't fully understand everything in reserve.go yet. will take a look at it again tomorrow.

pkg/postage/batchstore/store.go

pkg/postage/batchservice/batchservice.go

pkg/postage/batchstore/reserve.go

pkg/node/node.go

pkg/postage/batchservice/batchservice_test.go

acud · 2021-03-16T07:48:15Z

pkg/postage/batchstore/export_test.go

+	"github.com/ethersphere/bee/pkg/postage"
+)
+
+// ChainStateKey is the localstore key for the chain state.


localstore? also, name of variable is wrong

sry yes state store

pkg/postage/batchstore/export_test.go

acud · 2021-03-16T08:18:43Z

pkg/postage/batchstore/reserve_test.go

+				n := rand.Intn(len(batches))
+				b, err = bStore.Get(batches[n])
+				if err != nil {
+					if errors.Is(storage.ErrNotFound, err) {


the other way around. errors.Is(err, storage.ErrNotFound). Is this actually hit?

acud · 2021-03-16T08:20:05Z

pkg/postage/batchstore/reserve_test.go

+	t.Helper()
+	// we cannot  use the mock statestore here since the iterator is not giving the right order
+	// must use the leveldb statestore
+	dir, err := ioutil.TempDir("", "batchstore_test")


use t.TempDir

acud · 2021-03-16T08:23:12Z

pkg/postage/batchstore/reserve_test.go

+}
+
+func nextChainState(bStore postage.Storer) (*postage.ChainState, error) {
+	cs := bStore.GetChainState()


use t.Helper() and fail directly in the helper, it will make the tests more readable

acud · 2021-03-16T08:23:29Z

pkg/postage/batchstore/reserve_test.go

+// the following assumptions are tested on each modification of the batches (top up, depth increase, price change)
+// - reserve exceeds capacity
+// - value-consistency of unreserved POs
+func checkReserve(bStore postage.Storer, unreserved map[string]uint8) (uint8, error) {


acud · 2021-03-16T08:24:00Z

pkg/postage/batchstore/store.go

 	"math/big"

 	"github.com/ethersphere/bee/pkg/postage"
 	"github.com/ethersphere/bee/pkg/storage"
 )

 const (
-	batchKeyPrefix = "batchKeyPrefix"
-	stateKey       = "stateKey"
+	batchKeyPrefix  = "batch"


very generic key names that might conflict with others. can you add a bit more text here?

ralph-pichler

looks better. I do find the code to be quite tricky to understand though. More high-level documentation at the beginning might be useful, especially one that uses the same terminology as the code below (e.g. the docs at reserve.go don't even mention inner / outer, what properties should hold about them, etc.). I think this would also be quite useful to the auditors in the future.

ralph-pichler · 2021-03-15T09:51:00Z

pkg/postage/batchservice/batchservice.go

 	if err != nil {
 		return fmt.Errorf("put: %w", err)
 	}

-	svc.logger.Debugf("topped up batch id %s with %v", hex.EncodeToString(b.ID), b.Value)
+	svc.logger.Debugf("topped up batch id %s from %v to %v", hex.EncodeToString(b.ID), b.Value, normalisedBalance)


should this not be %d instead if %v?

ralph-pichler · 2021-03-15T14:06:11Z

pkg/postage/batchstore/reserve.go

+		if b.Value.Cmp(until) >= 0 {
+			return true, nil
+		}
+		//


please add some comments here

pkg/postage/batchstore/reserve.go

zelig

LGTM cannot approve

zelig · 2021-03-27T01:27:06Z

pkg/postage/batchstore/export_test.go

+	"github.com/ethersphere/bee/pkg/postage"
+)
+
+// ChainStateKey is the localstore key for the chain state.


zelig · 2021-03-27T01:29:43Z

pkg/postage/batchstore/reserve_test.go

+	t.Helper()
+	// we cannot  use the mock statestore here since the iterator is not giving the right order
+	// must use the leveldb statestore
+	dir, err := ioutil.TempDir("", "batchstore_test")


zelig · 2021-03-27T01:35:20Z

pkg/postage/batchstore/reserve_test.go

+	return depthValueTuple{depth: d, value: v}
+}
+
+type batchValueTuple struct {


you can just remove 'Tuple' fromall names

but that would force me to rename the function calls to create the instances to be renamed in a different way, since the same name of the type cannot be redeclared as a function (that's the reason why i use this naming convention)

ralph-pichler · 2021-03-31T08:55:07Z

pkg/postage/batchstore/store.go

 	"math/big"

 	"github.com/ethersphere/bee/pkg/postage"
 	"github.com/ethersphere/bee/pkg/storage"
 )

 const (
-	batchKeyPrefix = "batchKeyPrefix"
-	stateKey       = "stateKey"
+	batchKeyPrefix  = "batch"


these should still be changed before merge, prefix with postage_, batchstore_ or something

ralph-pichler · 2021-03-31T09:05:58Z

pkg/postage/batchstore/reserve.go

+		}
+		// if multiplier == 0 && batch value >= inner
+		if multiplier == 0 && b.Value.Cmp(s.rs.Inner) >= 0 {
+			multiplier = 1


This section still would benefit from more comments. Explain why are we setting the multiplier to that.

ralph-pichler · 2021-03-31T09:10:51Z

pkg/postage/batchstore/reserve.go

+	if err != nil {
+		return err
+	}
+	// set inner/outer to total if total is greater


This comment is not correct. It either keeps inner the same (if inner == 0 || inner > total) or sets it to total+1 otherwise.

postage: new pkg for postage stamps, uploader stamping (#890) * postage: new pkg for postage stamps, uploader stamping * postage: amount->value, blockNumber big.Int-> uint64, stamp only has batch ID, not Batch * postage: fix godoc and copyright * postage, swarm: - swarm.Stamp as an interface - add postage/testing for mock Stamps - fix Stamp MarshalBinary to allow nil batch id and signature - add StampSize const * postage: heed review feedback Co-authored-by: acud <12988138+acud@users.noreply.github.com> Storage incentives: add stamper putter to api Postage BatchStore and BatchService (#1070) Co-authored-by: zelig <viktor.tron@gmail.com> add normalisedBalance to updater interface (#1108) make value the normalised balance (#1111) postage: add event listener (#1099) Wire up postage stamp syncing (#1114) localstore, shed: persist stamps (#1116) add --postage to beeinfra.sh setup pullsync, pushsync: add postage stamps (#1117) postage: add create endpoint (#1142) retrieve erc20 address from postage contract (#1169) postage: check balance before attempting stamp creation (#1177) postage: fix bucket depth (#1178) api: use hex encoding in postage api (#1179) increase page size (#1182) postage: handle bucket depth error in api (#1183) localstore: attach stamp to outgoing chunk (#1192) update postage stamp contract addresses for new token (#1208) batchstore: reserve (#1262) * postage/batchstore: reserve logic Co-authored-by: acud <12988138+acud@users.noreply.github.com> stamp support in storage and protocols (#1321) api: endpoints for stamp issuers (#1535) retrieval: add stamps (#1552) localstore reserve logic (#1322) Co-authored-by: acud <12988138+acud@users.noreply.github.com>

zelig requested review from ralph-pichler and acud February 16, 2021 10:00

zelig self-assigned this Feb 16, 2021

bee-runner bot added the pull-request label Feb 16, 2021

zelig force-pushed the postage-batchstore-reserve branch from 59780e7 to bb01ce9 Compare February 16, 2021 12:26

zelig force-pushed the postage-batchstore-reserve branch from bb01ce9 to 059b920 Compare February 24, 2021 01:31

zelig added the ready for review The PR is ready to be reviewed label Feb 24, 2021

acud suggested changes Feb 25, 2021

View reviewed changes

acud reviewed Mar 1, 2021

View reviewed changes

pkg/postage/batchstore/store.go Outdated Show resolved Hide resolved

pkg/postage/batchstore/store.go Show resolved Hide resolved

acud reviewed Mar 1, 2021

View reviewed changes

pkg/storage/mock/batchstore/batchstore.go Outdated Show resolved Hide resolved

acud reviewed Mar 1, 2021

View reviewed changes

pkg/storage/mock/batchstore/batchstore.go Outdated Show resolved Hide resolved

acud reviewed Mar 2, 2021

View reviewed changes

pkg/postage/batchstore/reserve_test.go Outdated Show resolved Hide resolved

acud approved these changes Mar 2, 2021

View reviewed changes

ralph-pichler reviewed Mar 2, 2021

View reviewed changes

ralph-pichler approved these changes Mar 3, 2021

View reviewed changes

ralph-pichler reviewed Mar 3, 2021

View reviewed changes

pkg/postage/batchstore/reserve.go Outdated Show resolved Hide resolved

ralph-pichler suggested changes Mar 3, 2021

View reviewed changes

pkg/postage/batchstore/reserve.go Outdated Show resolved Hide resolved

ralph-pichler reviewed Mar 5, 2021

View reviewed changes

pkg/postage/batchstore/reserve.go Outdated Show resolved Hide resolved

zelig force-pushed the postage-batchstore-reserve branch 2 times, most recently from 41f4502 to fab6980 Compare March 13, 2021 07:36

zelig requested a review from acud March 14, 2021 09:26

zelig force-pushed the postage-batchstore-reserve branch from fab6980 to 88f49a2 Compare March 14, 2021 13:24

postage/batchstore: reserve logic

f652df3

zelig force-pushed the postage-batchstore-reserve branch from 88f49a2 to f652df3 Compare March 14, 2021 13:29

zelig requested a review from ralph-pichler March 14, 2021 20:52

acud reviewed Mar 16, 2021

View reviewed changes

ralph-pichler reviewed Mar 16, 2021

View reviewed changes

acud force-pushed the postage-batchstore-reserve branch from 02e5ea3 to 261602d Compare March 26, 2021 15:01

batchstore: document tests and cleanups

a572579

acud force-pushed the postage-batchstore-reserve branch from 261602d to a572579 Compare March 26, 2021 15:57

vandot and others added 4 commits March 26, 2021 16:58

add --no-install-recommends to apt-get install (#1438)

7bff569

lint

6875797

wtf windows

a2007f7

fix listener test

45ecae7

zelig commented Mar 27, 2021

View reviewed changes

comment

42e9327

ralph-pichler reviewed Mar 31, 2021

View reviewed changes

address ralphs comments

8e7bfee

ralph-pichler approved these changes Mar 31, 2021

View reviewed changes

acud merged commit 341242a into storage-incentives Mar 31, 2021

acud deleted the postage-batchstore-reserve branch March 31, 2021 10:09

acud restored the postage-batchstore-reserve branch April 8, 2021 16:59

acud deleted the postage-batchstore-reserve branch June 19, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batchstore: reserve #1262

batchstore: reserve #1262

zelig commented Feb 16, 2021 •

edited

Loading

acud commented Feb 23, 2021

acud Feb 23, 2021

zelig Mar 1, 2021

acud Mar 1, 2021

ralph-pichler left a comment

acud Mar 16, 2021

zelig Mar 16, 2021

zelig Mar 27, 2021

acud Mar 16, 2021

zelig Mar 16, 2021

acud Mar 16, 2021

zelig Mar 27, 2021

acud Mar 16, 2021

acud Mar 16, 2021

acud Mar 16, 2021

ralph-pichler left a comment

ralph-pichler Mar 15, 2021

ralph-pichler Mar 15, 2021

zelig left a comment

zelig Mar 27, 2021

zelig Mar 27, 2021

zelig Mar 27, 2021

acud Mar 31, 2021

ralph-pichler Mar 31, 2021

ralph-pichler Mar 31, 2021

ralph-pichler Mar 31, 2021

+              	})
+              }
+              //

batchstore: reserve #1262

batchstore: reserve #1262

Conversation

zelig commented Feb 16, 2021 • edited Loading

acud commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralph-pichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralph-pichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Feb 16, 2021 •

edited

Loading