Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: postage stamp indexing #1625

Merged
merged 2 commits into from
Jun 11, 2021
Merged

feat: postage stamp indexing #1625

merged 2 commits into from
Jun 11, 2021

Conversation

zelig
Copy link
Member

@zelig zelig commented Apr 29, 2021

this PR extends postage stamps with indexes

  • indexes enable replacing stored chunks at the discretion of the owner: storer nodes store only the latest chunk stamped with the same stamp (same batch same index)
  • offer an easy way to catch overissuance as part of stamp validity check
  • makes it possible to limit spoofing density proofs by mining chunks to a batch
  • makes it possible to transfer ownership of arbitrary indexes

the index is a 64-bit serialisation of

  • bucket depth (uint8, 1st byte, <24)
  • bucket index (neighbourhood index, uint32 <2^depth, bytes 2-4)
  • and the within-bucket index (uint32 <2^(batchdepth-bucketdepth), bytes 5-8)

This change is Reviewable

@zelig zelig added in progress ongoing development , hold out with review on hold Temporarily halted by other development labels Apr 29, 2021
@zelig zelig self-assigned this Apr 29, 2021
@zelig zelig mentioned this pull request Apr 29, 2021
pkg/localstore/mode_put.go Outdated Show resolved Hide resolved
pkg/postage/stampissuer.go Outdated Show resolved Hide resolved
pkg/postage/stampissuer.go Outdated Show resolved Hide resolved
@istae istae removed the on hold Temporarily halted by other development label May 31, 2021
@istae istae self-assigned this Jun 1, 2021
@istae istae force-pushed the stamp-indexing branch 2 times, most recently from 82d3d58 to 7884099 Compare June 1, 2021 19:31
@IxaBrjnko
Copy link

Yay!! Thank you <3

pkg/localstore/mode_put.go Outdated Show resolved Hide resolved
pkg/localstore/export.go Show resolved Hide resolved
pkg/api/postage_test.go Outdated Show resolved Hide resolved
pkg/localstore/mode_put.go Outdated Show resolved Hide resolved
@ralph-pichler
Copy link
Member

rebase on master, then change https://github.com/ethersphere/bee/blob/master/.github/workflows/beekeeper.yml#L16 to 0.2.0 to run integration tests with the new contract.

@istae
Copy link
Member

istae commented Jun 3, 2021

rebase on master, then change https://github.com/ethersphere/bee/blob/master/.github/workflows/beekeeper.yml#L16 to 0.2.0 to run integration tests with the new contract.

Ok! Thanks.

@acud acud force-pushed the stamp-indexing branch 3 times, most recently from 6ea0564 to 62502f9 Compare June 3, 2021 15:32
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 17 of 33 files at r1, 1 of 2 files at r2, 1 of 10 files at r4, 15 of 19 files at r7, 12 of 12 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @esadakar and @zelig)


pkg/localstore/gc.go, line 184 at r9 (raw file):

			return 0, false, err
		}
		err = db.postageIndexIndex.DeleteInBatch(batch, item)

need to add a test for the index, to see that the entry gets removed when GCing


pkg/localstore/localstore.go, line 511 at r9 (raw file):

		},
		DecodeValue: func(keyItem shed.Item, value []byte) (e shed.Item, err error) {
			e = keyItem

this should not be here, there are other occurrences of this usage in this file and they should also be removed


pkg/localstore/mode_put_test.go, line 494 at r9 (raw file):

	ts := time.Now().Unix()

	modes := []storage.ModePut{storage.ModePutRequest, storage.ModePutRequestPin, storage.ModePutSync, storage.ModePutUpload, storage.ModePutUploadPin}

nit, this can be pulled out to a package-level variable


pkg/localstore/mode_set_test.go, line 113 at r9 (raw file):

				for _, ch := range chs {
					wantErr := leveldb.ErrNotFound
					_, err := db.retrievalDataIndex.Get(addressToItem(ch.Address()))

this is not needed since index counts are tested below


pkg/localstore/mode_set_test.go, line 119 at r9 (raw file):

					// access index should not be set
					_, err = db.retrievalAccessIndex.Get(addressToItem(ch.Address()))

this is not needed since index counts are tested below


pkg/localstore/mode_set_test.go, line 131 at r9 (raw file):

			for _, ch := range chs {
				newPullIndexTest(db, ch, 0, leveldb.ErrNotFound)(t)

this is not needed since we test this anyway below


pkg/postage/stamp.go, line 71 at r9 (raw file):

// MarshalBinary gives the byte slice serialisation of a stamp:
// batchID[32]|index[32]|SignatureUser[65]|SignatureOwner[65].

comment incorrect


pkg/postage/stamper.go, line 48 at r9 (raw file):

	toSign, err := toSignDigest(addr.Bytes(), st.issuer.batchID, index, ts)
	if err != nil {
		return nil, err

if there was an error here, we need to think about decrementing the issuer count


pkg/postage/listener/listener_test.go, line 41 at r9 (raw file):

			amount:           big.NewInt(42),
			normalisedAmount: big.NewInt(43),
			// bucketDepth:      10,

commented code can be removed

Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 33 files at r1, 3 of 10 files at r3.
Reviewable status: 41 of 47 files reviewed, 9 unresolved discussions (waiting on @acud and @zelig)

Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r10.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acud and @zelig)


pkg/localstore/localstore.go, line 511 at r9 (raw file):

Previously, acud (acud) wrote…

this should not be here, there are other occurrences of this usage in this file and they should also be removed

Done.


pkg/localstore/mode_put_test.go, line 494 at r9 (raw file):

Previously, acud (acud) wrote…

nit, this can be pulled out to a package-level variable

Done.


pkg/localstore/mode_set_test.go, line 113 at r9 (raw file):

Previously, acud (acud) wrote…

this is not needed since index counts are tested below

Done.


pkg/localstore/mode_set_test.go, line 119 at r9 (raw file):

Previously, acud (acud) wrote…

this is not needed since index counts are tested below

Done.


pkg/localstore/mode_set_test.go, line 131 at r9 (raw file):

Previously, acud (acud) wrote…

this is not needed since we test this anyway below

Done.


pkg/postage/stamp.go, line 71 at r9 (raw file):

Previously, acud (acud) wrote…

comment incorrect

Done.


pkg/postage/listener/listener_test.go, line 41 at r9 (raw file):

Previously, acud (acud) wrote…

commented code can be removed

Done.

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 6 files at r10, 1 of 3 files at r11, 2 of 2 files at r13, 1 of 1 files at r15, 3 of 3 files at r16, 2 of 2 files at r17.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zelig)

@acud acud added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Jun 4, 2021
@acud acud requested a review from istae June 4, 2021 14:32
Copy link
Member Author

@zelig zelig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @zelig)


pkg/postage/stamp.go, line 71 at r17 (raw file):

// MarshalBinary gives the byte slice serialisation of a stamp:
// batchID[32]|index[8]|Signature[65].

comment still missing timestamp field


pkg/postage/stamp_test.go, line 118 at r17 (raw file):

}

func newStamp(t *testing.T) *postage.Stamp {

is this not the same as under postage/testing?


pkg/postage/stampissuer.go, line 68 at r17 (raw file):

// indexToBytes creates an uint64 index from
// - bucket depth (uint8, 1st byte, <24)

this comment is outdated, the bucket depth itself is no longer part of this serialisation

Copy link
Contributor

@aloknerurkar aloknerurkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @zelig)

Copy link
Contributor

@aloknerurkar aloknerurkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @zelig from 3 discussions.
Reviewable status: 47 of 50 files reviewed, 1 unresolved discussion (waiting on @acud, @esadakar, and @zelig)


pkg/postage/stamp.go, line 71 at r17 (raw file):

Previously, zelig (Viktor Trón) wrote…

comment still missing timestamp field

Done


pkg/postage/stamp_test.go, line 118 at r17 (raw file):

Previously, zelig (Viktor Trón) wrote…

is this not the same as under postage/testing?

Done


pkg/postage/stampissuer.go, line 68 at r17 (raw file):

Previously, zelig (Viktor Trón) wrote…

this comment is outdated, the bucket depth itself is no longer part of this serialisation

Done

@ralph-pichler
Copy link
Member

change

  • GoerliPostageStampContractAddress in listener.go to 0x621e455C4a139f5C4e4A8122Ce55Dc21630769E4
  • GoerliStartBlock to 4933174

@istae
Copy link
Member

istae commented Jun 9, 2021

change

  • GoerliPostageStampContractAddress in listener.go to 0x621e455C4a139f5C4e4A8122Ce55Dc21630769E4
  • GoerliStartBlock to 4933174

Done!

@acud acud changed the title postage stamps with indexes feat: postage stamp indexing Jun 11, 2021
@acud acud merged commit 3709726 into master Jun 11, 2021
@acud acud deleted the stamp-indexing branch June 11, 2021 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants