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

Future PRs #14

Open
jnewbery opened this issue Jul 8, 2019 · 15 comments

Comments

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Jul 8, 2019

Please comment in this issue to request PRs to be covered in future review club meetings.

The table in this description will be updated as PRs are added to the schedule.

PR Area Link Host Comment
Adding random tests against a naive implementation libsecp bitcoin-core/secp256k1#641 elichai
transaction fees in getblock RPC bitcoin/bitcoin#16083 Still WIP
Separate settings merging from parsing Config bitcoin/bitcoin#15934
Mempool: rework rebroadcast logic to improve privacy Mempool/wallet bitcoin/bitcoin#16698 amiti Still a WIP. Discuss this PR when it's ready for review
Multiprocess build support Build bitcoin/bitcoin#16367 Focus mainly on the testing process, not reviewing makefiles!
Don't query all DNS seeds at once p2p bitcoin/bitcoin#15558
@ariard

This comment has been minimized.

Copy link

@ariard ariard commented Aug 14, 2019

bitcoin/bitcoin#16367 I think multiprocess build support need to be tested by a lot of people on different os/distros, would be cool if attendees try to build and see if instructions are straightforward enough

@MarcoFalke

This comment has been minimized.

Copy link
Contributor

@MarcoFalke MarcoFalke commented Aug 27, 2019

bitcoin/bitcoin#16698

@mzumsande

This comment has been minimized.

Copy link
Contributor

@mzumsande mzumsande commented Aug 28, 2019

I would find bitcoin/bitcoin#16702 interesting.

@michaelfolkson

This comment has been minimized.

Copy link

@michaelfolkson michaelfolkson commented Sep 1, 2019

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?

@MarcoFalke

This comment has been minimized.

Copy link
Contributor

@MarcoFalke MarcoFalke commented Sep 3, 2019

@jnewbery

This comment has been minimized.

Copy link
Contributor Author

@jnewbery 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

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Sep 17, 2019

bitcoin/bitcoin#16202 Refactor network message deserialization

bitcoin/bitcoin#16401 Package relay

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Sep 30, 2019

Possibly bitcoin/bitcoin#15558 Don't query all DNS seeds at once (p2p). Despite only changing a few lines of code, the PR has a fair amount of discussion and a possible bug found post-merge -- or not?

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Oct 3, 2019

Proposed a twist on #15558 above in #38.

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Oct 10, 2019

bitcoin/bitcoin#16981 Improve runtime performance of --reindex ... looks ready for review now. WIP removed yesterday.

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Oct 10, 2019

Maybe one of the BIP174 PRs after that:

bitcoin/bitcoin#17034 Bip174 extensions

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

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Oct 10, 2019

bitcoin/bitcoin#16841 Replace GetScriptForWitness with GetScriptForDestination

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

@jonatack

This comment has been minimized.

Copy link
Contributor

@jonatack jonatack commented Oct 10, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Contributor Author

@jnewbery jnewbery commented Oct 11, 2019

bitcoin/bitcoin#15845 wallet: Fast rescan with BIP157 block filters. It's +375/-32, but the vast majority is in new test code. Changes to product code are only ~+50.

@jnewbery

This comment has been minimized.

Copy link
Contributor Author

@jnewbery jnewbery commented Oct 28, 2019

@ryanofsky suggests https://github.com/bitcoin/bitcoin/pull/16442/commits and says "PR is surprisingly not that complicated, and you could even limit review to the first 5 commits since the real functionality is all there (excluding tests, caching, tweaks)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.