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.

Sign up for GitHub

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

Jump to bottom

Future PRs #14

Closed
jnewbery opened this issue Jul 8, 2019 · 52 comments
Closed

Future PRs #14

jnewbery opened this issue Jul 8, 2019 · 52 comments

Comments

@jnewbery
Copy link
Contributor

jnewbery commented Jul 8, 2019 •
edited
Loading

Please comment in this issue to request PRs to be covered in future review club meetings.
@ariard
Copy link

ariard commented Aug 14, 2019 •
edited by jnewbery
Loading

@maflcko
Copy link
Contributor

maflcko commented Aug 27, 2019 •
edited by jnewbery
Loading

@mzumsande
Copy link
Contributor

mzumsande commented Aug 28, 2019 •
edited
Loading

@michaelfolkson
Copy link
Contributor

When should we make a decision by? Need to give time for people to read in advance. Any of the above would be good for me. Are the PRs in the table ones that are due to happen but just haven't been scheduled yet?

@maflcko
Copy link
Contributor

maflcko commented Sep 3, 2019

@jnewbery

@jnewbery
Copy link
Contributor Author

jnewbery commented Sep 3, 2019

@MarcoFalke

bitcoin/bitcoin#16698

Still a WIP. We can cover this when it's no longer WIP and is ready for code review

@mzumsande

I would find bitcoin/bitcoin#16702 interesting.

That PR is quite large (+559/-77) and requires a lot of contextual knowledge. I think it's a bit too ambitious for review club I'm afraid. (review club is mostly for new reviewers)

@MarcoFalke / @michaelfolkson

When should we make a decision by? Need to give time for people to read in advance.

Ideally they'd be on the website with notes and questions one week in advance. That's always been my intention, but I haven't managed that the last couple of weeks. Any help organizing the review club would be greatly appreciated!

@jonatack
Copy link
Contributor

jonatack commented Sep 17, 2019 •
edited by jnewbery
Loading

@jonatack
Copy link
Contributor

jonatack commented Sep 30, 2019 •
edited by jnewbery
Loading

@jonatack
Copy link
Contributor

jonatack commented Oct 3, 2019

Proposed a twist on #15558 above in #38.

@jonatack
Copy link
Contributor

jonatack commented Oct 10, 2019 •
edited by jnewbery
Loading

@jonatack
Copy link
Contributor

jonatack commented Oct 10, 2019 •
edited by jnewbery
Loading

Maybe one of the BIP174 PRs after that:

@jonatack
Copy link
Contributor

jonatack commented Oct 10, 2019 •
edited by jnewbery
Loading

@jonatack
Copy link
Contributor

jonatack commented Oct 10, 2019 •
edited by jnewbery
Loading

@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 11, 2019 •
edited
Loading

@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 28, 2019 •
edited
Loading

@michaelfolkson
Copy link
Contributor

+1 for bitcoin/bitcoin#14430 Add more property based tests for basic bitcoin data structures

Author of PR (Chris Stewart) answered my StackExchange question on property based tests here: https://bitcoin.stackexchange.com/questions/91780/property-based-tests-have-been-added-to-bitcoin-core-what-are-they-and-could-th

@jnewbery
Copy link
Contributor Author

bitcoin/bitcoin#17034 Bip174 extensions
bitcoin/bitcoin#16463 [BIP 174] Implement serialization support for GLOBAL_XPUB field

One PSBT is enough. I've added 17034.

@jnewbery
Copy link
Contributor Author

@jonatack

bitcoin/bitcoin#16841 Replace GetScriptForWitness with GetScriptForDestination

This looks like a simple refactor. I don't think there's much of substance to discuss here.

bitcoin/bitcoin#16910 wallet: reduce loading time by using unordered maps... data structures and flame graphs FTW :)

This seems like a scary change for a 5% performance improvement (over something that usually takes a handful of seconds at startup). I'm not inclined to review this myself - is there a reason you think we should spend time on this?

@jnewbery
Copy link
Contributor Author

I've put a check against all suggested PRs that I've added to the table above, or that we've covered in a previous review club, or that have been merged/closed for a while.

@jnewbery
Copy link
Contributor Author

Just added bitcoin/bitcoin#17954 and bitcoin/bitcoin#16902

@maflcko
Copy link
Contributor

maflcko commented Mar 3, 2020 •
edited by jnewbery
Loading

@jnewbery
Copy link
Contributor Author

jnewbery commented Mar 4, 2020

bitcoin/bitcoin#18234 could be an interesting candidate

I agree. Would you like to host?

@jonatack
Copy link
Contributor

jonatack commented Mar 5, 2020

@ariard
Copy link

ariard commented May 28, 2020

I would be glad to host BIP324 implementation : bitcoin/bitcoin#18242, there is a lot of interesting context.

@jonatack
Copy link
Contributor

I would be glad to host BIP324 implementation : bitcoin/bitcoin#18242, there is a lot of interesting context.

I agree, and it would be a good follow-up on https://bitcoincore.reviews/16202. Though that PR might be a bit on the large side for the review club.

@fjahr
Copy link
Contributor

fjahr commented May 29, 2020

I would like to host bitcoin/bitcoin#19105, ideally on 10th of June.

@jnewbery
Copy link
Contributor Author

I would be glad to host BIP324 implementation : bitcoin/bitcoin#18242, there is a lot of interesting context.

@ariard It'd be great to have you host a session on BIP324, but I agree with Jon that this PR is too large for PR review club. Is there some subset or prerequisite PR we could review instead. I think we should aim for a PR that's no more than 100 lines.

I'm happy for you to host on June 17th if you can find something suitable.

@ariard
Copy link

ariard commented May 29, 2020

@jnewbery yes we can focus only on new serializer/deserializer and not cover the re-keying part. So from commit "Expose MAC length in chacha_poly_aead_h" to "Add BIP324 v2 transport serializer and deserializer".

This scope sounds reasonable ?

June 17th sounds great!

@sipa
Copy link
Contributor

sipa commented May 29, 2020

I'm happy to host one on bitcoin/bitcoin#18468

@jnewbery
Copy link
Contributor Author

I would like to host bitcoin/bitcoin#19105, ideally on 10th of June.

@fjahr Yes please!

we can focus only on new serializer/deserializer and not cover the re-keying part. So from commit "Expose MAC length in chacha_poly_aead_h" to "Add BIP324 v2 transport serializer and deserializer".

@ariard even that is ~400 lines, so it's a big ask for attendees to review. I think if we do host a meeting on that, we need to make sure we have really clear notes and that we publish them a bit earlier than normal to give people time to review.

Are you able to provide notes & questions by next Friday (June 5th)?

I'm happy to host one on bitcoin/bitcoin#18468

@sipa That'd be amazing! We'll schedule it for June 24th.

@ariard
Copy link

ariard commented May 29, 2020

@jnewbery Yes will send clear, concise, with-context, notes before Friday 12pm UTC :)

@jnewbery
Copy link
Contributor Author

Thanks @ariard! Let me know if I can help with the notes in any way.

@fjahr
Copy link
Contributor

fjahr commented Jun 27, 2020

I would like to do bitcoin/bitcoin#19328 next. This is an early part of coinstatsindex but without the hashing algo so it can be merged without it. It has a good size and should be pretty accessible for beginners. There is a little overlap with #19055 but these parts in particular have undergone a lot of changes (partly based on feedback from the review club) and since the session on #19055 mostly discussed the hashing algo and the high level benefits of the whole project, I think going in depth into this part would still be worthwhile.

@pinheadmz
Copy link
Contributor

I reviewed https://github.com/bitcoin/bitcoin/pull/18267/files (signet) and would be happy to host unless @jonatack or @kallewoof has dibs?
I'd like to send signet BTC to participants who get questions right 😄 ...and compiled the branch 😄

@jonatack
Copy link
Contributor

jonatack commented Jul 25, 2020 •
edited
Loading

@pinheadmz Yes, I proposed to run a signet session a few months back, because it's a nice tour of many different components of the codebase. Our worry was that it's quite a few LOC for a review club PR. IDK, maybe a particular interesting commit or subset of commits? Edit: I don't have dibs! The meeting hours don't work well for me ATM, nor for @kallewoof, I believe.

@kallewoof
Copy link

Would love to see a signet session, but @ajtowns is working on some cool changes, so my suggestion is to wait until that's done. :)

@jnewbery
Copy link
Contributor Author

The whole signet PR is too much to cover in a review club meeting, but as @jonatack points out, we could cover a commit or subset of commits. I'd be happy for any of you to host that once aj's rework is ready for review.

@pinheadmz
Copy link
Contributor

Would be a useful session just to explore the changes to chainparams.cpp? It would be a good review on what defines a network and signet in particular.

@jnewbery
Copy link
Contributor Author

@pinheadmz - sounds good. Let's try to get it scheduled once aj has made his changes.

@michaelfolkson
Copy link
Contributor

These suggestions for future ones look great. Can we add Taproot functional tests to this list that Greg S has agreed to host?

@michaelfolkson
Copy link
Contributor

For broader topics (e.g. Signet, Core process separation, Core test framework) that could do with a longer session than a hour on a single PR and could be aided with visuals/screenshare we are looking for future topics for presentations and Socratic Seminars at London BitDevs (online). Please contact me if interested :)

@jnewbery jnewbery mentioned this issue Sep 4, 2020
Merged
@jnewbery
Copy link
Contributor Author

I think it'd be good to do a session on the cryptography in bitcoin/bitcoin#19055. We did a previous session on that PR when it contained a lot more, but it's been reduced to just the C++ implementation of MuHash and I think we could do a session focused on that.

@jonatack
Copy link
Contributor

Yes. Another cryptography PR that's been on my review list since February is bitcoin/bitcoin#18014.

@ryanofsky
Copy link
Contributor

@jnewbery I'd like to do multiprocess bitcoin/bitcoin#10102 and wallet transaction state bitcoin/bitcoin#21206 review clubs at some point. Also if you're still looking for a host next week (saw mentioned in IRC) I could host for one of these or for a different PR

@maflcko
Copy link
Contributor

maflcko commented Nov 18, 2021 •
edited
Loading

Next week is taken ;) #420

@ryanofsky
Copy link
Contributor

Just saw a missed message in IRC from @glozow, but I'd be happy to host a review club for bitcoin/bitcoin#10102 anytime (since bitcoin/bitcoin#21206 has already been merged).

@maflcko
Copy link
Contributor

maflcko commented Dec 7, 2021

I am unsure, but I think it is also allowed to host already merged prs for post-merge review clubs?

@jnewbery
Copy link
Contributor Author

jnewbery commented Dec 7, 2021

I think it is also allowed to host already merged prs for post-merge review clubs?

Of course it is. Not only allowed, but encouraged.

@glozow
Copy link
Contributor

glozow commented Dec 8, 2021

@ryanofsky would you like to host on December 22nd? Either #21206 or #10102 would be great

@ryanofsky
Copy link
Contributor

I'd do #10102 December 22. Will write up some notes and questions and open a PR to add them to the review club website, if that is the next step.

@glozow
Copy link
Contributor

glozow commented Dec 8, 2021

@ryanofsky great! Thank you 🤗

@jnewbery jnewbery closed this as completed Jan 8, 2022
@jnewbery
Copy link
Contributor Author

jnewbery commented Jan 8, 2022

Closing. Please use the irc channel to request future PR review club meetings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
13 participants
@kallewoof @sipa @jnewbery @fjahr @pinheadmz @jonatack @maflcko @ryanofsky @michaelfolkson @ariard @glozow @mzumsande and others