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

Add Notes and Questions for 18267 (signet) #250

Merged
merged 7 commits into from
Sep 4, 2020

Conversation

pinheadmz
Copy link
Contributor

Added preliminary notes and questions for signet review club. Lemme know wha chu think! @jonatack @jnewbery @kallewoof

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Thanks @pinheadmz, looking forward to this session! A few suggestions below.

_posts/2020-09-09-#18267.md Show resolved Hide resolved
_posts/2020-09-09-#18267.md Outdated Show resolved Hide resolved
_posts/2020-09-09-#18267.md Outdated Show resolved Hide resolved
_posts/2020-09-09-#18267.md Outdated Show resolved Hide resolved
_posts/2020-09-09-#18267.md Outdated Show resolved Hide resolved
_posts/2020-09-09-#18267.md Outdated Show resolved Hide resolved
@jonatack
Copy link
Contributor

jonatack commented Sep 4, 2020

@kallewoof you may want to have a look 🚀

@jnewbery
Copy link
Contributor

jnewbery commented Sep 4, 2020

I've pushed a commit that addresses jon's comments (thanks!) and does some minor formatting.

@jnewbery
Copy link
Contributor

jnewbery commented Sep 4, 2020

I think this is a good start. A couple of high-level points:

  1. This PR is larger than we'd usually tackle in review club (+482/-44 LOC). As we discussed here: Future PRs #14 (comment), I think we need to explicitly focus the review club on a subset of the commits, or this will be overwhelming for participants.
  2. It'd be good to refer to the code in the Notes section to point out particular areas to pay attention to. You don't need to spoon feed the participants, just give them pointers for what to look at in detail so we have stuff to talk about in the meeting.

I'd like to get (1) addressed before merging this (so participants know what to review over the weekend). (2) can be added later if necessary.

@pinheadmz
Copy link
Contributor Author

Wow you guys work fast :-) I'll address the remaining comments now

@pinheadmz
Copy link
Contributor Author

Ok pushed a series of new commits. I split it up so I can further address feedback by rebasing single hunks.
p.s. @jnewbery commit message in b611a7d has typo in PR number :-)

@jnewbery
Copy link
Contributor

jnewbery commented Sep 4, 2020

p.s. @jnewbery commit message in b611a7d has typo in PR number :-)

Fixed. Thank you!

@jnewbery
Copy link
Contributor

jnewbery commented Sep 4, 2020

Looks good to me. Thanks @pinheadmz !

I pushed one final commit to make the links stable.

@jnewbery jnewbery merged commit 34b618c into bitcoin-core-review-club:master Sep 4, 2020
@jonatack
Copy link
Contributor

jonatack commented Sep 4, 2020

Nice 👍 will tweet and toot the news 🚀

@kallewoof
Copy link

Apologies about the bad timing; we merged some changes that cut out some edge cases but at the cost of another reset. (An overflow issue due to the min difficulty being too low caused the reset as the genesis block changed.)

@jnewbery
Copy link
Contributor

jnewbery commented Sep 8, 2020

Thanks Kalle. I've updated the branch here: https://github.com/bitcoin-core-review-club/bitcoin/tree/pr18267 (we create a tag in our own repository so links remain stable after the meeting). I don't think the change invalidates any of the notes or questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants