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

verifcid: introduce and integrate Allowlist interface #407

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Jul 9, 2023

The successor of #281.

@Wondertan Wondertan requested a review from a team as a code owner July 9, 2023 17:03
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #407 (3b6a259) into main (b96767c) will increase coverage by 0.02%.
The diff coverage is 82.22%.

❗ Current head 3b6a259 differs from pull request most recent head a0a69e0. Consider uploading reports for the commit a0a69e0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   49.78%   49.80%   +0.02%     
==========================================
  Files         248      249       +1     
  Lines       29941    29972      +31     
==========================================
+ Hits        14906    14928      +22     
- Misses      13610    13617       +7     
- Partials     1425     1427       +2     
Files Changed Coverage Δ
provider/reprovider.go 69.06% <14.28%> (-0.97%) ⬇️
verifcid/allowlist.go 66.66% <66.66%> (ø)
blockservice/blockservice.go 77.09% <95.74%> (+3.52%) ⬆️
verifcid/cid.go 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

@Wondertan Wondertan force-pushed the verifcid/validator-interface branch from 5c14093 to ccda271 Compare July 9, 2023 17:25
@Wondertan Wondertan changed the title refactor(verifcid): introduce HashValidator interface verifcid: introduce Allowlist interface Jul 9, 2023
verifcid/allowlist.go Outdated Show resolved Hide resolved
verifcid/validate.go Outdated Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Jul 11, 2023

@Wondertan can you give me your opinion on 8e41b53 please ?

@Jorropo Jorropo force-pushed the verifcid/validator-interface branch from 1f57332 to 8e41b53 Compare July 11, 2023 14:22
Copy link
Member Author

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

@Jorropo, I am fond of the simplifications you made! Removal of ValidateCID is neat, and glad you didn't add all the Blake family in the switch case.

I still don't really get the use case you see for the OverridingAllowlist, though. Something like NewOverridingAllowlist(DefaultAllowlist, customset)?

Would be also nice to add unit test for overriding

verifcid/validate.go Outdated Show resolved Hide resolved
verifcid/validate.go Outdated Show resolved Hide resolved
verifcid/validate.go Outdated Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Jul 11, 2023

Something like NewOverridingAllowlist(DefaultAllowlist, customset)?

Exactly

Would be also nice to add unit test for overriding

Yes

@Jorropo Jorropo self-requested a review July 11, 2023 16:10
@BigLep
Copy link
Contributor

BigLep commented Jul 11, 2023

2023-07-11 maintainer conversation: since the number of consumers of low, we should break away from globals/defaults. @Jorropo can provide more details.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

I am onboard with fixing this, thx for doing it, but I'll be busy this week. Should make it for kubo 0.22, hopefully.

verifcid/validate.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

I won't have time this and the following week either. Mainly due to crypto events in Paris

@Wondertan
Copy link
Member Author

Rebased

@Wondertan Wondertan requested a review from Jorropo August 2, 2023 21:24
@Wondertan
Copy link
Member Author

I've implemented the requested changes. One last thing to consider is perhaps renaming the pkg to something more suitable.

@Wondertan Wondertan changed the title verifcid: introduce Allowlist interface verifcid: introduce and integrate Allowlist interface Aug 2, 2023
@BigLep BigLep mentioned this pull request Aug 3, 2023
@Wondertan
Copy link
Member Author

@Jorropo, gentle ping

@Jorropo
Copy link
Contributor

Jorropo commented Aug 16, 2023

I've noticed we couldn't have a blockservice that is both writethrough and with an allowlist so I refactored it to the functional option pattern.
I've also added tests.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Thx, this looks good to me, I just have minor questions.

verifcid/cid.go Show resolved Hide resolved
verifcid/allowlist.go Outdated Show resolved Hide resolved
@Jorropo Jorropo force-pushed the verifcid/validator-interface branch from 5f1f415 to 836a67c Compare August 16, 2023 04:36
@Wondertan Wondertan requested a review from Jorropo August 16, 2023 07:54
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM thx ❤️

@Wondertan
Copy link
Member Author

There is one test failure, which seems unrelated and likely a flake:

--- FAIL: TestMfsStress (1.06s)
      mfs_test.go:889: file already exists

@Jorropo
Copy link
Contributor

Jorropo commented Aug 16, 2023

Actually let's not embed it, it's gonna confuse ADI frameworks. Also you didn't embeded in the implementation (just interface) and this is breaking the tests.

@Wondertan
Copy link
Member Author

Wondertan commented Aug 16, 2023

I realized that a bit late and fixed it now. Please take a look at the new proper commit, and if we like the result we can keep it, if not, I'll revert.

@Jorropo Jorropo force-pushed the verifcid/validator-interface branch from ae7441a to 836a67c Compare August 16, 2023 08:19
@Jorropo
Copy link
Contributor

Jorropo commented Aug 16, 2023

I realized that a bit late and fixed it now. Please take a look at the commit, and if we like the result we can keep it, if not, I'll revert.

I force pushed it away, this is a type problem.
Basically frameworks like fx are gonna look at blockservice and may choose to plug it inside anyone that needs an Allowlist, including blockservice. So it's gonna try to instanciate a blockservice in order to instanciate a blockservice, hopefully it errors when it realise the dependency graph is cyclic, or it eats of all your memory explode and catch fire, who knows ?

@Jorropo Jorropo force-pushed the verifcid/validator-interface branch from 3b6a259 to a0a69e0 Compare August 16, 2023 08:23
@Wondertan
Copy link
Member Author

FWIW, FX realizes dep cycles, but I see your point and even cycle error would be annoying to deal with, and simple fx.Provide would need tags or additional fx shenanigans to make it work. Let's keep it then how I made it originally.

@Wondertan
Copy link
Member Author

Wondertan commented Aug 16, 2023

My last take is that to fully finish this Allowlist interface with default impl should move to go-multihash and ValidateCid with errors should live inside blockservice pkg. Would love to know your thoughts on that.

@Jorropo Jorropo merged commit dd32d67 into ipfs:main Aug 16, 2023
11 checks passed
@Jorropo
Copy link
Contributor

Jorropo commented Aug 16, 2023

If you want to make an other PR about moving stuff around why not, I'm not sure about putting in the blockservice tho, because some of my code use verifcid without using the blockservice.

If anything I think this would be worth moving into go-cid, this might break a lot of peoples tho.

@Wondertan
Copy link
Member Author

What's the ETA for the next release including this PR?

@Jorropo
Copy link
Contributor

Jorropo commented Aug 16, 2023

ipfs/kubo#10014 Currently September 10.

In theory we could make a Boxo release before a Kubo one, we recently got in the habits of keeping them in sync because it's convenient, but if you need this I don't see why not release boxo earlier.

@Wondertan
Copy link
Member Author

I would appreciate an earlier release, so we can finally migrate to boxo as part of our big IPFS deps bump PR

@Wondertan Wondertan deleted the verifcid/validator-interface branch August 16, 2023 16:17
@hacdias hacdias mentioned this pull request Aug 17, 2023
10 tasks
distractedm1nd added a commit to celestiaorg/celestia-node that referenced this pull request Sep 5, 2023
Thanks to ipfs/boxo#407, we can now fully
migrate to boxo and get rid of the oldest `replace` directive in our
go.mod for `verifcid`.


TODO: Issue about go-car/v2 migration

---------

Co-authored-by: Ryan <ryan@celestia.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants