From 0ec529468202503ea8fab7f8aa4c886376466636 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Mon, 5 Jun 2023 11:59:06 +0300 Subject: [PATCH 01/21] initialize structure for review club pages --- docs/review-club/README.md | 68 ++++++++++ docs/review-club/code-of-conduct.md | 81 ++++++++++++ docs/review-club/hosting.md | 195 ++++++++++++++++++++++++++++ docs/review-club/meetings/README.md | 0 4 files changed, 344 insertions(+) create mode 100644 docs/review-club/README.md create mode 100644 docs/review-club/code-of-conduct.md create mode 100644 docs/review-club/hosting.md create mode 100644 docs/review-club/meetings/README.md diff --git a/docs/review-club/README.md b/docs/review-club/README.md new file mode 100644 index 0000000000..1e2a276898 --- /dev/null +++ b/docs/review-club/README.md @@ -0,0 +1,68 @@ +### A weekly review club for BDK PRs + +What is this?  A weekly club for reviewing +Bitcoin Core PRs at **{{ site.meeting_time }} on {{ site.meeting_day }}s** in +{{ site.meeting_location }}. + +What's it for?  To help newer contributors +learn about the Bitcoin Core review process. The review club is *not* primarily +intended to help open PRs get merged. + +Who should take part?  Anyone who wants to +learn about contributing to Bitcoin Core. All are welcome to come and ask +questions! + +What's the benefit for participants? + Reviewing and testing PRs is the best way to start contributing to Bitcoin +Core, but it's difficult to know where to start. There are hundreds of open PRs, +many require a lot of contextual knowledge, and contributors and reviewers often +use unfamiliar terminology. The review club will give you the tools and +knowledge you need in order to take part in the [Bitcoin Core review +process](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review) +on GitHub. + +How do I take part? Just show up on IRC! See +[Attending your first PR Review Club](/your-first-meeting/) for more tips +on how to participate. To stay up to date on newly announced review clubs, +you can follow us on Twitter or via the Atom feed. + +Who runs this?  Upcoming meetings are +scheduled by {{ site.coordinator }}. +The meetings are hosted by a variety of Bitcoin Core contributors. See +some of our [previous hosts](/meetings-hosts/). + +## Upcoming Meetings + +We're always looking for interesting PRs to discuss in the review club and for +volunteer hosts to lead the discussion: + +- If there's a PR that you'd like to discuss in a future meeting, feel free to suggest it in the IRC channel. +- If you'd like to host a meeting, look at the [information for meeting + hosts](/hosting) and contact {{ site.coordinator_irc }} on IRC. + +## Recent Meetings + +See all [meetings](/meetings/). + +## Other Resources for New Contributors + +- Read the [Contributing to Bitcoin Core + Guide](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md). This + will help you understand the process and some of the terminology we use in + Bitcoin Core. +- Look at the [Good First + Issues](https://github.com/bitcoin/bitcoin/issues?q=is%3aissue+is%3aopen+label%3a%22good+first+issue%22) + and [Up For + Grabs](https://github.com/bitcoin/bitcoin/issues?utf8=%e2%9c%93&q=label%3a%22up+for+grabs%22) + list. +- Read the Bitcoin Core [Developer + Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md) + and [Productivity + Tips](https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md). +- Brush up on your C++. There are [many primers and reference manuals + available](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). +- There are some excellent blog posts on how to start contributing to Bitcoin Core: + - [A Gentle Introduction to Bitcoin Core Development (Jimmy Song)](https://bitcointechtalk.com/a-gentle-introduction-to-bitcoin-core-development-fdc95eaee6b8) + - [Contributing to Bitcoin Core - a Personal Account (John Newbery)](https://bitcointechtalk.com/contributing-to-bitcoin-core-a-personal-account-35f3a594340b) + - [Onboarding to Bitcoin Core (Amiti Uttarwar)](https://medium.com/@amitiu/onboarding-to-bitcoin-core-7c1a83b20365) + - [How to Review Pull Requests in Bitcoin Core (Jon Atack)](https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core) diff --git a/docs/review-club/code-of-conduct.md b/docs/review-club/code-of-conduct.md new file mode 100644 index 0000000000..c7223627da --- /dev/null +++ b/docs/review-club/code-of-conduct.md @@ -0,0 +1,81 @@ +# Code of Conduct + +The PR Review Club is designed to be a safe, respectful, and productive +environment for everyone. We are dedicated to providing a harassment-free +experience for all participants regardless of race, gender, age, sexual +orientation, disability, physical appearance, national origin, ethnicity, or +religion. This code of conduct is a guide for the behavior we support and don't +support at our club meetings. + +#### Conduct + +Examples of behavior that contribute to a positive environment include: + +- Using welcoming and inclusive language +- Being respectful of differing viewpoints and experiences +- Gracefully accepting constructive criticism +- Focusing on what is best for the community +- Showing empathy towards other community members + +Examples of unacceptable behavior by participants include: + +- [Harassment](#harassment) +- Public promotional activities +- Questioning or challenging someone’s stated self-identity or chosen labels +- Making general statements about groups you do not belong to +- Incitement of violence towards any individual +- Unauthorized publication of private information or communication +- Unwelcome comments regarding a person’s lifestyle choices and practices, + including those related to food, health, parenting, relationships, drugs, and + employment +- Gratuitous or off-topic sexual images or behaviour + +#### Harassment + +Harrassment can take many forms. Examples include, but are not limited to: + +- Threats of violence +- Deliberate intimidation +- Stalking or following +- Unwanted recording, including logging online activity for harassment purposes +- Sustained disruption of discussion +- Unwelcome sexual attention +- Pattern of inappropriate social contact, such as requesting/assuming + inappropriate levels of intimacy with others +- Continued one-on-one communication after requests to cease +- Offensive comments or personal questions relating to gender, gender identity + and expression, sexual orientation, disability, mental illness, + neuro(a)typicality, physical appearance, body size, age, race, national origin, + ethnic origin, nationality, immigration status, language, religion or lack + thereof, or other identity marker + +#### If you see a violation + +1. Notify the participant they are in violation of the code of conduct and ask + them to stop their behavior. That participant should comply immediately. + +2. If the participant does not correct the issue or if you’re uncomfortable + speaking up, contact the administrator. When reporting, please include any + relevant details, links, screenshots, context that may be used to better + understand and resolve the situation. + +3. As soon as available, an administrator will join, identify themselves, and + take further action. These actions could include: + + - A warning + - Removal from the club’s channel for the duration of that week’s meeting + - Being banned from all future club meetings + +We reserve the right to reject any report we believe to have been made in bad +faith. Reports intended to silence legitimate criticism may be deleted without +response. + +We will respect confidentiality requests for the purpose of protecting victims +of abuse. At our discretion, we may publicly name a person about whom we’ve +received harassment complaints, or privately warn third parties about them, if +we believe that doing so will increase the safety of The PR Review Club or the +general public. We will not name harassment victims without their affirmative +consent. + +Thank you for your help in keeping our club welcoming, respectful, and friendly +to all participants. diff --git a/docs/review-club/hosting.md b/docs/review-club/hosting.md new file mode 100644 index 0000000000..eff395f933 --- /dev/null +++ b/docs/review-club/hosting.md @@ -0,0 +1,195 @@ +# Hosting a BDK Review Club Meeting + +## How to Choose a PR + +- The purpose of PR Review Club is to help new contributors learn about the + BDK codebase and the process of contributing to the project. Select a PR + that is likely to be instructive or interesting. Don't select a PR just because + you want it to be merged soon! You may even select a PR that is already merged. + +- The PRs discussed at review club should span a range of complexity + and difficulty levels, in order to cater to attendees with a variety of + different experience levels. Some attendees have only just cloned the BDK repo for the first time, and some have been contributing for months or + years. Having some meetings on basic PRs and others on more complex PRs ensures + that there's something for everyone. + +- Generally, try to pick PRs that don't require a very high level of contextual + knowledge, since it's difficult to introduce a lot of concepts in a one hour + meeting. + +- The appropriate amount of code change to discuss depends on the complexity of + the code. It might be fine to discuss 10-20 lines of code if the change is + very subtle or has complex interactions. If the change is more straightforward, + then it might be fine to discuss 100+ lines of code change. If the PR seems too + large to cover in a meeting, consider only discussing a subset of the commits, + or breaking up the discussion over multiple meetings. + +- It's fine to discuss a PR that you opened yourself. After all, you probably + know more about it than anyone else! + +## Before the Meeting + +- Consider contacting the PR author to let them know that you're going to + talk about their PR in the review club. They'll probably be happy that you're + interested in their PR and may be prepared to answer questions and give + you more context. They may even want to join the meeting themselves. + +- Post an announcement for your PR as soon as you can - preferably 2 weeks + before the meeting. See the [Making a New Post](#making-a-new-post) section + below for how to make an announcement post. + +- Post notes and questions on the Friday the week before the meeting. + That gives attendees time to review the PR and prepare for the meeting. Please + open your PR by that Thursday to allow time for maintainers to review and + merge it before the weekend. + +- Prepare a range of questions from basic to advanced. The review club is a way + to help new developers learn. There should be something for everyone. + +- In the notes and meeting, don't just talk about the mechanical code changes. + Other important aspects to consider are: + + - The motivation for the PR. + - The historical context of the PR (why things in the code are the way they + are currently, and what PRs have touched that part of the code base in the past). + - How to review and test the changes. + - Advantages and disadvantages of this approach compared to alternative approaches. + - Potential bugs reviewers might want to look out for. + - Fun Rust things. + +- If you refer to yourself in the notes, use third person narrative + (e.g. your name or GitHub profile) rather than first person ("I"). + +- Don't spoon-feed the attendees. Give them pointers for + where to look in the codebase/GitHub/documentation, and let them do their + own research from there. + +- When linking to code, use stable links that won't break later: + + - for links to code in the master branch, link to a recent commit hash in + the master branch of bitcoindevkit/bdk, e.g. + [https://github.com/bitcoindevkit/bdk/blob/49632405/crates/chain/src/chain_oracle.rs#L9](https://github.com/bitcoindevkit/bdk/blob/49632405/crates/chain/src/chain_oracle.rs#L9). + You can use a short (8 character) commit id. + + **REASON**: linking directly to master + isn't stable because line numbers/files change over time. + +- Thorough preparation will really pay off. Taking the time to deeply + understand the PR will make you feel more relaxed during the meeting. + +- If you find yourself struggling with writing the notes and questions, having + them ready by Friday, or choosing what areas to focus on, ask the review club + maintainers for help. + +- It can be useful to write out some anticipated answers/links/references + before the meeting so you don't have to type them in the moment. + +#### Making a New Post + +[Fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks) the website repository [located on GitHub](https://github.com/bitcoindevkit/bitcoindevkit.org). + +To make a new post, run the following command from the website directory (replace YYYY-MM-DD with next meeting data and PR_NUMBER with the number of the PR being reviewed): + +```shell +cd ./docs/review-club/meetings && touch YYYY-MM-DD-#PR_NUMBER.md +``` + +The above command will create a markdown file in the +`./docs/review-club/meetings` directory of the website repository + +At the top of that file. Add the following frontmatter content + +``` +``` + +Inspire yourself from the previous meetings markdown document structure to +structure your meeting notes. + +## In the Meeting + +- Come with the mindset that the attendees are people who are as smart as you, + but don't have the context that you do in BDK or + the BDK codebase. + +- Start the meeting with `#startmeeting` and "hi" from everyone so you know + who's at their keyboard and paying attention. + +- It's nice to remind people of some of our meeting conventions at the start: + + - Attendees don't have to ask to ask a question (e.g. "I have a question + about x but I don't know if it's on-topic?"). They should just go ahead and + ask. If it's off-topic, the host will say so. + + - The host is there to help moderate, not to lead. Attendees don't need to wait + for the host to ask a specific question — they can just jump in at any point. + +- A quick poll at the start ("Did everyone get a chance to review the PR? How + about a quick y/n from everyone") establishes who will probably be + asking/answering the more interesting questions. + +- Move quickly at the start of the meeting to get to the interesting parts of + the PR as soon as possible — preferably within 2 or 3 minutes. + +- Don't worry if people haven't had a chance to review the PR yet. This + is a voluntary meeting and most people have a lot of other commitments. + +- Very open questions to the group (e.g. "Does everyone know how tx gossip works + in Bitcoin?") don't get a good response because all the attendees are waiting for + someone else to answer. Instead try to make the questions focused on the + PR or change set that is being reviewed (e.g. "how does commit X change the + way bitcoind gossips transactions?"). Try to phrase questions positively + ("please describe how X works") rather than negatively ("Does anyone not know + how X works?"). + +- Once the meeting has reached the challenging questions, + it sometimes feels like there are long stretches of silence and no one + is out there. Be patient. It takes people a bit of time to formulate their + thoughts and type them out. + +- Be encouraging! For many people, asking questions or volunteering an answer + can be intimidating, even in the pseudonymous comfort of IRC. People are + there to learn. Try to create an environment where they feel safe to ask any + questions and where they can attempt to answer without fear. + +- Keep an eye on the clock, and try to wrap up the session with `#endmeeting` at the end of the + hour. Even if you're happy to continue beyond the hour, some of the attendees + might have hard stops. + +- Have fun, and pat yourself on the back 🚀 + +## After the Meeting + +Let one of the review club maintainers know that the meeting is over. They'll +take care of updating the website. + +#### Adding logs to the review club Page + +_This process is done by the review club maintainers_ + +- Uncomment the `## Meeting Log` markup in the meeting post and copy-paste the + meeting log into it. Meeting logs should be copied exactly, but an `## + Erratum` section can be added to correct factual errors. + + - To sanitize logs exported from IRCCloud (ensure UTC timezone is selected + during export), use the following command on the unzipped .txt file: + ```sh + cat \#bdk-pr-reviews.txt \ + | sed -n '/#startmeeting/h;//!H;$!d;x;//p' \ + | sed '/#endmeeting/q' \ + | sed '/[⇐→]/d' \ + | cut -c 13-17,22- \ + > sanitized_log.txt + ``` + +- Change the `status` of the meeting post from `upcoming` to `past`. + ```diff + -status: upcoming + +status: past + ``` + +- Add the first 7 characters of the PR commit hash at HEAD to the meeting post. + This adds a link to the tagged branch on the review club repo. + ```diff + -commit: + +commit: eebaca7 + ``` diff --git a/docs/review-club/meetings/README.md b/docs/review-club/meetings/README.md new file mode 100644 index 0000000000..e69de29bb2 From 49ac19866961d2c54c477c82eaeb25e128c15513 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 7 Jun 2023 09:42:28 +0300 Subject: [PATCH 02/21] Add link to review club page --- docs/.vuepress/config.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/.vuepress/config.js b/docs/.vuepress/config.js index ac44442906..224f2c5089 100644 --- a/docs/.vuepress/config.js +++ b/docs/.vuepress/config.js @@ -106,6 +106,10 @@ module.exports = { text: 'Blog', link: '/blog/' }, + { + text: 'Review Club', + link: '/review-club/' + }, { text: 'Discord', link: discordUrl From 1be7febfdee589cc5681958a74bb72e41498fa27 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 7 Jun 2023 10:00:51 +0300 Subject: [PATCH 03/21] Add layout for ReviewClub pages --- .../.vuepress/components/ReviewClubLayout.vue | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 docs/.vuepress/components/ReviewClubLayout.vue diff --git a/docs/.vuepress/components/ReviewClubLayout.vue b/docs/.vuepress/components/ReviewClubLayout.vue new file mode 100644 index 0000000000..8667dd03e2 --- /dev/null +++ b/docs/.vuepress/components/ReviewClubLayout.vue @@ -0,0 +1,64 @@ + + + + + \ No newline at end of file From ad9e15a53186d0bb6b1203792de7bafb21469a6e Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 7 Jun 2023 10:04:49 +0300 Subject: [PATCH 04/21] add title and layout in frontmatter --- docs/review-club/README.md | 7 +++++++ docs/review-club/code-of-conduct.md | 5 +++++ docs/review-club/hosting.md | 5 +++++ docs/review-club/meetings/README.md | 4 ++++ 4 files changed, 21 insertions(+) diff --git a/docs/review-club/README.md b/docs/review-club/README.md index 1e2a276898..533d2f2c30 100644 --- a/docs/review-club/README.md +++ b/docs/review-club/README.md @@ -1,3 +1,8 @@ +--- +layout: ReviewClubLayout +title: BDK Weekly PR review club +--- + ### A weekly review club for BDK PRs What is this?  A weekly club for reviewing @@ -66,3 +71,5 @@ See all [meetings](/meetings/). - [Contributing to Bitcoin Core - a Personal Account (John Newbery)](https://bitcointechtalk.com/contributing-to-bitcoin-core-a-personal-account-35f3a594340b) - [Onboarding to Bitcoin Core (Amiti Uttarwar)](https://medium.com/@amitiu/onboarding-to-bitcoin-core-7c1a83b20365) - [How to Review Pull Requests in Bitcoin Core (Jon Atack)](https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core) + + diff --git a/docs/review-club/code-of-conduct.md b/docs/review-club/code-of-conduct.md index c7223627da..8d0fc548d1 100644 --- a/docs/review-club/code-of-conduct.md +++ b/docs/review-club/code-of-conduct.md @@ -1,3 +1,8 @@ +--- +layout: ReviewClubLayout +title: Code Of Conduct +--- + # Code of Conduct The PR Review Club is designed to be a safe, respectful, and productive diff --git a/docs/review-club/hosting.md b/docs/review-club/hosting.md index eff395f933..b2e294bda9 100644 --- a/docs/review-club/hosting.md +++ b/docs/review-club/hosting.md @@ -1,3 +1,8 @@ +--- +layout: ReviewClubLayout +title: Hosting Review Club +--- + # Hosting a BDK Review Club Meeting ## How to Choose a PR diff --git a/docs/review-club/meetings/README.md b/docs/review-club/meetings/README.md index e69de29bb2..22057bfcf8 100644 --- a/docs/review-club/meetings/README.md +++ b/docs/review-club/meetings/README.md @@ -0,0 +1,4 @@ +--- +layout: ReviewClubLayout +title: Meeting Notes +--- \ No newline at end of file From f0aaae1d723dad59abccbeefa411c0843689a517 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:33:50 +0300 Subject: [PATCH 05/21] Define custom global props and other helpers Aside from the custom properties add filter for date formatting and helper methods for sorting meeting pages according to the most recent. --- docs/.vuepress/enhanceApp.js | 38 ++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 docs/.vuepress/enhanceApp.js diff --git a/docs/.vuepress/enhanceApp.js b/docs/.vuepress/enhanceApp.js new file mode 100644 index 0000000000..16f51bba6c --- /dev/null +++ b/docs/.vuepress/enhanceApp.js @@ -0,0 +1,38 @@ +export default ({ Vue }) => { + Vue.filter('formatDate', dateString => { + const date = new Date(dateString); + const options = { day: '2-digit', month: 'short', year: 'numeric' }; + return date.toLocaleDateString('en-US', options); + }); + + // Create a mixin + const customPropertyMixin = { + created() { + this.$meetingTime = "15:00 UTC"; + this.$meetingDay = "Thursday"; + this.$meetingPlace = ""; + }, + }; + + // Apply the mixin globally + Vue.mixin(customPropertyMixin); + + // Create a mixin + const customMethodMixin = { + methods: { + $getLastFourMeetings(pages) { + let pastMeetings = pages.filter(page => page.frontmatter.status === 'past' && page.frontmatter.layout === 'ReviewClubMeetingLayout'); + pastMeetings.sort((a, b) => { + const dateA = new Date(a.frontmatter.date); + const dateB = new Date(b.frontmatter.date); + return dateB - dateA; + }); + return pastMeetings.length > 4? pastMeetings.slice(0, 4) : pastMeetings; + }, + }, + }; + + // Apply the mixin globally + Vue.mixin(customMethodMixin); + +}; \ No newline at end of file From fcfb9fe770d8441636d700f7d71366fe1710b77c Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:47:53 +0300 Subject: [PATCH 06/21] Add the he package for escaping html chars --- package-lock.json | 29 ++++++++++++++++------------- package.json | 3 ++- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2e0ce795d4..c6074e594e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,7 +15,8 @@ "start-server-and-test": "2.0.0", "vue": "2.7.14", "vue-server-renderer": "2.7.14", - "vuepress": "1.9.9" + "vuepress": "1.9.9", + "he": "^1.2.0" } }, "node_modules/@ampproject/remapping": { @@ -11661,12 +11662,6 @@ "javascript-stringify": "^1.6.0" } }, - "node_modules/markdown-it-container": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/markdown-it-container/-/markdown-it-container-2.0.0.tgz", - "integrity": "sha512-IxPOaq2LzrGuFGyYq80zaorXReh2ZHGFOB1/Hen429EJL1XkPI3FJTpx9TsJeua+j2qTru4h3W1TiCRdeivMmA==", - "dev": true - }, "node_modules/markdown-it-emoji": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/markdown-it-emoji/-/markdown-it-emoji-1.4.0.tgz", @@ -18977,6 +18972,12 @@ "markdown-it-container": "^2.0.0" } }, + "node_modules/vuepress-plugin-container/node_modules/markdown-it-container": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/markdown-it-container/-/markdown-it-container-2.0.0.tgz", + "integrity": "sha512-IxPOaq2LzrGuFGyYq80zaorXReh2ZHGFOB1/Hen429EJL1XkPI3FJTpx9TsJeua+j2qTru4h3W1TiCRdeivMmA==", + "dev": true + }, "node_modules/vuepress-plugin-disqus": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/vuepress-plugin-disqus/-/vuepress-plugin-disqus-0.2.0.tgz", @@ -29714,12 +29715,6 @@ } } }, - "markdown-it-container": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/markdown-it-container/-/markdown-it-container-2.0.0.tgz", - "integrity": "sha512-IxPOaq2LzrGuFGyYq80zaorXReh2ZHGFOB1/Hen429EJL1XkPI3FJTpx9TsJeua+j2qTru4h3W1TiCRdeivMmA==", - "dev": true - }, "markdown-it-emoji": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/markdown-it-emoji/-/markdown-it-emoji-1.4.0.tgz", @@ -35653,6 +35648,14 @@ "requires": { "@vuepress/shared-utils": "^1.2.0", "markdown-it-container": "^2.0.0" + }, + "dependencies": { + "markdown-it-container": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/markdown-it-container/-/markdown-it-container-2.0.0.tgz", + "integrity": "sha512-IxPOaq2LzrGuFGyYq80zaorXReh2ZHGFOB1/Hen429EJL1XkPI3FJTpx9TsJeua+j2qTru4h3W1TiCRdeivMmA==", + "dev": true + } } }, "vuepress-plugin-disqus": { diff --git a/package.json b/package.json index 23bbc94781..2e7e7648d3 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,7 @@ "start-server-and-test": "2.0.0", "vue": "2.7.14", "vue-server-renderer": "2.7.14", - "vuepress": "1.9.9" + "vuepress": "1.9.9", + "he": "^1.2.0" } } From ad8a314f41f1722a01c3236a760b596aef62b0cc Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:49:09 +0300 Subject: [PATCH 07/21] Define a parsing/rendering irc wrapper --- docs/.vuepress/components/IRCWRAPPER.vue | 125 +++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 docs/.vuepress/components/IRCWRAPPER.vue diff --git a/docs/.vuepress/components/IRCWRAPPER.vue b/docs/.vuepress/components/IRCWRAPPER.vue new file mode 100644 index 0000000000..132c27a755 --- /dev/null +++ b/docs/.vuepress/components/IRCWRAPPER.vue @@ -0,0 +1,125 @@ + + + + + \ No newline at end of file From 0c1823bbf8fb96451f8d9e8b06426cd0965a9c57 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:49:39 +0300 Subject: [PATCH 08/21] Add header layout for review club site --- .../.vuepress/components/ReviewClubHeader.vue | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/.vuepress/components/ReviewClubHeader.vue diff --git a/docs/.vuepress/components/ReviewClubHeader.vue b/docs/.vuepress/components/ReviewClubHeader.vue new file mode 100644 index 0000000000..045e341d58 --- /dev/null +++ b/docs/.vuepress/components/ReviewClubHeader.vue @@ -0,0 +1,58 @@ + + + + + + \ No newline at end of file From d99a2e5073c7757d3da3615b862aa04ffe85ed1d Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:50:34 +0300 Subject: [PATCH 09/21] Create header for meeting pages --- .../components/ReviewClubMeetingHeader.vue | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 docs/.vuepress/components/ReviewClubMeetingHeader.vue diff --git a/docs/.vuepress/components/ReviewClubMeetingHeader.vue b/docs/.vuepress/components/ReviewClubMeetingHeader.vue new file mode 100644 index 0000000000..0431f88876 --- /dev/null +++ b/docs/.vuepress/components/ReviewClubMeetingHeader.vue @@ -0,0 +1,31 @@ + + + + + + \ No newline at end of file From 16cbceae43a90ac5e9b3ea4e77682451116acbf8 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:51:06 +0300 Subject: [PATCH 10/21] Create meeting page layout --- .../components/ReviewClubMeetingLayout.vue | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 docs/.vuepress/components/ReviewClubMeetingLayout.vue diff --git a/docs/.vuepress/components/ReviewClubMeetingLayout.vue b/docs/.vuepress/components/ReviewClubMeetingLayout.vue new file mode 100644 index 0000000000..29941e725b --- /dev/null +++ b/docs/.vuepress/components/ReviewClubMeetingLayout.vue @@ -0,0 +1,64 @@ + + + + + \ No newline at end of file From cbcc177554342909ecc4fe1dfcf7653a21c3cfd1 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:51:56 +0300 Subject: [PATCH 11/21] Modify the core template to fit BDK --- docs/review-club/README.md | 82 +++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/docs/review-club/README.md b/docs/review-club/README.md index 533d2f2c30..b68b48d4f1 100644 --- a/docs/review-club/README.md +++ b/docs/review-club/README.md @@ -3,70 +3,88 @@ layout: ReviewClubLayout title: BDK Weekly PR review club --- -### A weekly review club for BDK PRs +### A fortnightly review club for BitcoinDevKit(BDK) PRs -What is this?  A weekly club for reviewing -Bitcoin Core PRs at **{{ site.meeting_time }} on {{ site.meeting_day }}s** in -{{ site.meeting_location }}. +What is this?  A fortnightly review club for BitcoinDevKit (BDK) PRs at **{{ $meetingTime }} on {{ $meetingDay }}s** in the #bdk-pr-reviews IRC channel on +{{ $meetingPlace }}. What's it for?  To help newer contributors -learn about the Bitcoin Core review process. The review club is *not* primarily +learn about the BDK review process. The review club is *not* primarily intended to help open PRs get merged. Who should take part?  Anyone who wants to -learn about contributing to Bitcoin Core. All are welcome to come and ask +learn about contributing to BDK. All are welcome to come and ask questions! -What's the benefit for participants? - Reviewing and testing PRs is the best way to start contributing to Bitcoin -Core, but it's difficult to know where to start. There are hundreds of open PRs, -many require a lot of contextual knowledge, and contributors and reviewers often -use unfamiliar terminology. The review club will give you the tools and -knowledge you need in order to take part in the [Bitcoin Core review -process](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review) -on GitHub. - How do I take part? Just show up on IRC! See -[Attending your first PR Review Club](/your-first-meeting/) for more tips +[Attending your first PR Review Club](/review-club/your-first-meeting/) for more tips on how to participate. To stay up to date on newly announced review clubs, -you can follow us on Twitter or via the Atom feed. +you can follow us on Twitter. Who runs this?  Upcoming meetings are -scheduled by {{ site.coordinator }}. -The meetings are hosted by a variety of Bitcoin Core contributors. See +scheduled by {{ "site.coordinator" }}. +The meetings are hosted by a variety of BDK contributors. See some of our [previous hosts](/meetings-hosts/). ## Upcoming Meetings +
+

Meetings

+
+
    + +
+
+
+ We're always looking for interesting PRs to discuss in the review club and for volunteer hosts to lead the discussion: - If there's a PR that you'd like to discuss in a future meeting, feel free to suggest it in the IRC channel. - If you'd like to host a meeting, look at the [information for meeting - hosts](/hosting) and contact {{ site.coordinator_irc }} on IRC. + hosts](/review-club/hosting/) and contact us on [Discord](https://discord.gg/dstn4dQ). + + ## Recent Meetings -See all [meetings](/meetings/). +
+

Meetings

+
+
    + +
+
+
+ +See all [meetings](/review-club/meetings/). ## Other Resources for New Contributors -- Read the [Contributing to Bitcoin Core - Guide](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md). This - will help you understand the process and some of the terminology we use in - Bitcoin Core. +- Read the [Contributing to BDK + Guide](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md). This + will help you understand the process and some of the terminology we use in BDK. - Look at the [Good First Issues](https://github.com/bitcoin/bitcoin/issues?q=is%3aissue+is%3aopen+label%3a%22good+first+issue%22) and [Up For Grabs](https://github.com/bitcoin/bitcoin/issues?utf8=%e2%9c%93&q=label%3a%22up+for+grabs%22) list. -- Read the Bitcoin Core [Developer - Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md) - and [Productivity - Tips](https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md). -- Brush up on your C++. There are [many primers and reference manuals - available](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). -- There are some excellent blog posts on how to start contributing to Bitcoin Core: +- Brush up on your Rust. There are [many primers and reference manuals + available](https://github.com/rust-unofficial/awesome-rust#resources) plus the [Book](https://doc.rust-lang.org/book/). +- There are some excellent blog posts on how to start contributing to Bitcoin: + - [Contributing to Bitcoin (Daniela Brozzoni)](https://danielabrozzoni.com/posts/contributing_to_oss/) - [A Gentle Introduction to Bitcoin Core Development (Jimmy Song)](https://bitcointechtalk.com/a-gentle-introduction-to-bitcoin-core-development-fdc95eaee6b8) - [Contributing to Bitcoin Core - a Personal Account (John Newbery)](https://bitcointechtalk.com/contributing-to-bitcoin-core-a-personal-account-35f3a594340b) - [Onboarding to Bitcoin Core (Amiti Uttarwar)](https://medium.com/@amitiu/onboarding-to-bitcoin-core-7c1a83b20365) From 8c64de60068c5bf56befaa941a59c460887ec704 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:52:28 +0300 Subject: [PATCH 12/21] Add instructions for attending review club --- docs/review-club/your-first-meeting.md | 62 ++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 docs/review-club/your-first-meeting.md diff --git a/docs/review-club/your-first-meeting.md b/docs/review-club/your-first-meeting.md new file mode 100644 index 0000000000..de7ff81229 --- /dev/null +++ b/docs/review-club/your-first-meeting.md @@ -0,0 +1,62 @@ +--- +layout: ReviewClubLayout +title: Attending your first PR Review Club +--- + +## Attending your first PR Review Club + +If you’re thinking about attending PR Review Club, welcome! We love new +participants. Below is information to help you get started as a first-timer. + +The club meets on {{ $meetingDay }}s at {{ $meetingTime }} in +{{ $meetingPlace }}. Every other week, a BDK +developer will host a 60-minute discussion on a BDK pull request. [The +code of conduct](/review-club/code-of-conduct) details the +behavior we expect from all participants. + +**If you’re new to IRC (Internet Relay Chat),** it has its +own little culture and etiquette. + +The basics: + +- Follow the Golden Rule: treat others as you’d like to be treated. +- Assume the best in others (tone is hard over text). +- Talk in full sentences, but don't stress too much about grammar or + capitalization. +- To respond directly to someone, start your message with their full + nick and a colon, like this: otherperson: thanks for the + help! There's no need to @ them. +- Just observing (sometimes called lurking) is completely fine. + +For more on this subject, check out [The Beginner’s guide to +IRC](https://fedoramagazine.org/beginners-guide-irc/). + +You’ll get the most out of Review Club if you’ve reviewed the PR going into the +meeting, but it’s definitely not mandatory in order to attend. You can always +show up as an observer. If you're worried (don't be!), read the meeting log of a +[past PR Review Club](/review-club/meetings/). You'll see it's +very casual. + +If you’d like to more actively participate, you should clone the [BDK +repository](https://github.com/bitcoindevkit/bdk). You should also check out and +build the PR branch locally and run all tests. Take time to review the code changes and +read the comments on that week’s PR. + +There are also notes and questions you can review and prepare. Make a note +of any questions you want to ask. + +The meeting will begin with **#startmeeting** and +“hi” from that week’s host. Feel free to respond with “hi” so people know +you’re at your keyboard and participating. The host will poll everyone on +whether they've reviewed the PR (y/n). Don't feel embarrassed to say "n". + +Questions may be asked at any time. If you’d like to ask a question, +there's no need to ask permission. If the question is off-topic, the host will kindly +let you know. The host will end the session with **#endmeeting** +at the end of the hour. Thanking the host at the end of the meeting is always +appreciated! + +Follow the [PR Review Club on Twitter]() for +announcements on that week’s PR or schedule changes. + +**We look forward to having you at the next PR Review Club!** \ No newline at end of file From bb92b1392ebdedb9767a6d3ac9134973dff9c11d Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:52:59 +0300 Subject: [PATCH 13/21] Modify code for rendering meetings page --- docs/review-club/meetings/README.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/review-club/meetings/README.md b/docs/review-club/meetings/README.md index 22057bfcf8..ee299f7ad5 100644 --- a/docs/review-club/meetings/README.md +++ b/docs/review-club/meetings/README.md @@ -1,4 +1,19 @@ --- layout: ReviewClubLayout title: Meeting Notes ---- \ No newline at end of file +--- + +
+

Meetings

+
+
    + +
+
+
From 468120a9b6242169d0adfa8a7c39fa44a2fd0af0 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:53:21 +0300 Subject: [PATCH 14/21] Add sample meeting post --- docs/review-club/meetings/2023-06-08-10.md | 254 +++++++++++++++++++++ 1 file changed, 254 insertions(+) create mode 100644 docs/review-club/meetings/2023-06-08-10.md diff --git a/docs/review-club/meetings/2023-06-08-10.md b/docs/review-club/meetings/2023-06-08-10.md new file mode 100644 index 0000000000..1225d9e41b --- /dev/null +++ b/docs/review-club/meetings/2023-06-08-10.md @@ -0,0 +1,254 @@ +--- +layout: ReviewClubMeetingLayout +date: 2023-03-22 +title: "Sample Meeting" +pr: https://github.com/bitcoindevkit/bdk/pull/1004 +authors: [dergoegge] +components: ["p2p", "validation"] +host: vladimirfomene +status: past +commit: 1429da5078c436f04203213133736605f907cb15 +commitLink: https://github.com/danielabrozzoni/bdk/commit/1429da5078c436f04203213133736605f907cb15 +--- + + + +## Notes + +- [BIP 141 + (SegWit)](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki) + introduced a new structure called a `witness` to each transaction. + Transactions' witnesses are commited to blocks separately from the + transaction merkle tree. Witnesses contain data required to check transaction + validity, but not required to determine transaction effects (output + consumption/creation). In other words, witnesses are used to validate the + blockchain state, not to determine what that state is. + + - Witnesses are commited to by placing the root of the *witness merkle tree* in + the block's coinbase transaction. By doing so, the witnesses are also commited to + in the transaction merkle tree through the coinbase transaction. Nesting the + witness commitment in the coinbase transaction was done to make SegWit + soft-fork compatible. + +- `Assume-valid` is a node setting that makes the node skip some transaction + validity checks (signature and script checks) prior to a pre-determined + "known to be good" block (assume-valid point). The default assume-valid point + is set by the developers and is updated every release, but users have the + ability to set their own assume-valid point through the `-assumevalid` + setting. + + - The assume-valid feature does not significantly change Bitcoin's security + assumptions. If developers (and everyone reviewing the code changes) were + to conspire with miners to build a more-work chain with invalid signatures + (and go undetected for weeks), and then include it as the default + assume-valid point, they could get the network to accept an invalid chain. + However, those same people already have that power by just changing the + code - which would be much less obvious. + ([quoted](https://bitcoin.stackexchange.com/questions/59940/what-are-the-trust-assumptions-in-assumed-valid-in-bitcoin-core-0-14)) + Additionally, as long as the full chain history remains available for + auditing it would be hard for such an attack to go unnoticed. + + - It is also important to note that the configured assume-valid point does not + dictate which chain a node follows. The node still does + Proof of Work checks, meaning that a large reorg would be able + to orphan (parts of) the assumed-valid chain. + +- Nodes in prune mode (enabled by the `-prune` setting) fully download and + validate the chain history to build a UTXO set enabling them to fully + validate any new transaction, but only store a (configurable) portion of the + recent history. + +- [PR #27050](https://github.com/bitcoin/bitcoin/pull/27050) proposes to skip + downloading the witnesses for blocks prior to the configured assume-valid + point, for nodes running in prune mode. The rationale for this change is that + pruned nodes currently download witnesses but then (prior to the assume-valid + point) don't validate them and delete them shortly after. So why not skip + downloading those witnesses and save some bandwidth? + +## Questions + +1. Did you review the PR? [Concept ACK, approach ACK, tested ACK, or + NACK](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review)? + What was your review approach? + +2. How much bandwidth is saved, i.e., + what is the cumulative size of all witness data up to block + `0000000000000000000013a20dcc8577282e1eabd430592bb8afdd5fe544c05a`? (Hint: + the `getblock` RPC returns the `size` and `strippedsize` (size excluding + witnesses) for each block). + +3. The end goal of the PR can be achieved with very few changes to the code + (ignoring edge case scenarios). It essentially only requires two changes, + one to the block request logic and one to block validation. Can you (in your + own words) describe these two changes in more detail? + +4. Without this PR, script validation is skipped under assume-valid, but other + checks that involve witness data are not skipped. What other witness related + checks exist as part of validation on master? + +5. With this PR, all additional witness related checks (Q4) will be skipped for + assumed-valid blocks. Is it ok to skip these additional checks? Why or why not? + +6. The PR does not include an explicit code change for skipping all the extra + checks from Q4. Why does that work out? + +7. [Peter Todd left a + comment](https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1423474935) + concerning a reduction in security with the changes made in the PR. Can you + in your own words summarize his concerns? Do you agree/disagree with them? + +## Meeting Log + + +
+12:59 < afig> sup yall
+13:00 < jb55> yo
+13:00 < jnewbery> hi
+13:00 < pinheadmz> hi
+13:00 < Lightlike> hello
+13:00 < merehap> hi
+13:00 < amiti> hi
+13:00 < b10c> hi
+13:01 < jonatack> Hi
+13:01 < jnewbery> Hi folks! Thanks for joining PR review club today.
+13:01 < jnewbery> Before we start talking about #15834, I wanted to poll for feedback on these meetings. Is there any way we could make them more useful?
+13:02 < jnewbery> I'm also taking suggestions for PRs to cover in future weeks. We don't have anything on the agenda yet.
+13:02 < JulioBarros> I think the chat interface is interesting but wonder if an audio or video format would be helpful.
+13:03 < peevsie> hi
+13:03 < Lightlike> just a small thing, could you update the old gist page (with the old agenda) to link to the new page, for the people who have bookmarked that.
+13:03 < aj> heyo
+13:04 < jnewbery> The reason I chose IRC is that it makes it easier for people who don't have somewhere to take a video/audio call to join, and for people who want to just dip in and out
+13:04 < merehap> I definitely prefer IRC over audio/video. Easier to reference the code for a few minutes without falling too far behind in the conversation.
+13:04 < jnewbery> also means we have a text log for people who can't join in real time
+13:05 < jnewbery> ok, I don't want to take too long on this. If you think of anything, please raise it here or just message me directly
+13:05 < jnewbery> OK, onto #15834 - Fix NOTFOUND bug and expire getdata requests for transactions. As usual, I'll give a quick summary before we open up for questions.
+13:05 < jb55> my only suggestion is that it might be neat to have a site where people can propose reviews and have people signup to them, with a calendar and everything
+13:05 < jb55> perhaps when this gets more popular...
+13:06 < jnewbery> jb55: until then, feel free to propose reviews in this channel
+13:06 < jnewbery> This PR is for changes to the "Net Processing" layer. That layer sits between "Net" (which manages connections to peers and is responsible for reading/writing bytes off the wire) and "Validation" (which is responsible for our node's view of the state of the blockchain and mempool).
+13:06 < jnewbery> "Net Processing" receives application level messages from peers, which can either be data messages (eg tx and block inventory) or control messages (eg gossiping addresses of peers or establishing new connections). The developer reference here: https://btcinformation.org/en/developer-reference#p2p-network is probably the best place for documentation on p2p messages.
+13:07 < jnewbery> Data messages are parsed and then passed up to validation for consideration for us to update mempool/chainstate.
+13:07 < jnewbery> This change is in the handling of the INV/GETDATA/TX message exchange, and is an attempt to fix a bug introduced in #14897 where our node could potentially stop asking peers to send us new transactions.
+13:08 < jnewbery> As it says in the PR description, this is a smaller alternative to https://github.com/bitcoin/bitcoin/pull/15776 , who's author is in this chat
+13:08 < jnewbery> aj was actually the first person to report the bug a couple of months ago
+13:09 < jnewbery> as a result of that, we reverted #14897 in the 0.18 release, but kept it in master with the aim of fixing the bug while still keeping the good things about 14897
+13:09 < jnewbery> I'll shut up now. Any questions?
+13:09 < iiiiiiiiiii> >a bug introduced in #14897 https://github.com/bitcoin/bitcoin/pull/14897
+13:09 < jb55> how would you find a bug like this? is there tracing diagnostics you can turn on to see these messages?
+13:10 < jnewbery> aj: do you want to comment on how you found the bug?
+13:10 < aj> so what happened is i ran "getmempoolinfo" and noticed that it was empty -- figured something weird was going on and mentioned it on irc
+13:10 < aj> at which point people said "no, nothing weird is going on, your node is just having problems" or something similar
+13:11 < aj> so i tried working out what was going on at that point, because my mempool had just like 12 tx's and wasn't getting any more
+13:11 < aj> eventually figured out how to turn net message debugging on to the live system, bitcoin-cli logging '["net"]' or similar
+13:12 < jnewbery> It's a bit scary that this was only discovered by chance. I think there's more testing we could be doing to increase our chances of catching things like this earlier on.
+13:12 < aj> that then showed that i was getting lots of INV messages from peers, telling me that new tx's were around, but i was never actually sending the GETDATA back to get the details of those transactions
+13:12 < jnewbery> that's a good tip. We have different categories of logging that can be enabled and disabled at run time. Use the `logging` RPC.
+13:12 < aj> i then tried staring at code to figure out what was wrong, and made guesses. also tried attaching gdb to the running process, but that caused it to crash, and when i restarted everything was fine
+13:13 < aj> talking about it on irc, people were wondering if i was connected to hostile peers that were deliberately attacking me; so i checked against gmaxwell's banlist, but didn't get any matches, and noted down the peers i was connected to
+13:13 < aj> (i run my node behind NAT and not on tor, so i've only got 8 outgoing peers, no incoming peers)
+13:14 < aj> once it restarted and was working fine, i manually reconnected to the peers that didn't work before, to check they did work now, and they did so i took that to mean it wasn't a case of hostile peers, but a bug on my end
+13:14 < jonatack> aj, did the custom logging you mentioned in your PR shine light too? https://github.com/bitcoin/bitcoin/pull/15776#issuecomment-483916240
+13:14 < Lightlike> In 15776, there is a rather special situation, with the parent of the tx already expired from the relay set, leading to NOTFOUND msg being sent. But wouldn't you need to have 100 different children tx, each with expired parents to reach MAX_PEER_IN_FLIGHT, multiplied by the number of peers (8) to be cut off completely. Is that not extremely unlikely?
+13:14 < aj> in the log of #15776 i think you can see some of my guesses and corrections to that
+13:15 < aj> Lightlike: it's cumulative, so you use up 1 slot with one tx, then it happens against and you use up another slot
+13:16 < aj> Lightlike: and it's not that the parent expires from the relay set, it's that you receive a child you don't have a parent for (because you've just restarted your node after being down for a while, eg), and then you ask the peer for the parent even though they never told you they had the parent, so it's not in the collection of tx's they'll relay to you even though it's in the mempool
+13:16 < merehap> Beyond more testing, is there something like using better concurrency primitives or better code structuring to reduce the likelihood of concurrency problems like these? I feel like I've seen several different concurrency bugs file against core. Since concurrency bugs are so difficult to discover and debug, it seems scary if there isn't a more systematic approach to preventing them.
+13:17 < merehap> filed* against
+13:17 < aj> Lightlike: (also, if the parent isn't in the mempool, but is in your utxo set, but the first two outputs from the transaction have already been spent, then you'll redundantly ask for the tx and get a notfound back)
+13:18 < jnewbery> Good context to know is that if a peer asks you for a tx with GETDATA, you'll only send that tx if it's in a data structure called MapRelay. You won't fish the tx out of the mempool if it's not in MapRelay
+13:18 < aj> jnewbery: (except you will if you're working with an SPV peer doing bloom filters)
+13:19 < jnewbery> ah, yes. Thanks!
+13:19 < aj> jnewbery: (there's always an exception. except when there isn't...)
+13:19 < jnewbery> merehap: I wouldn't really categorise this as a concurrency bug. This is more like a resource leak (where the resource is txs in flight from a peer)
+13:20 < MarcoFalke> Yeah, it is a logic bug. The code was written under the wrong assumptions (missing some "edge" cases)
+13:21 < pinheadmz> so the story is: INV(tx)-->    <--GETDATA(tx)   (tx is dropped from relayset)   NOTFOUND(tx)-->      ?
+13:21 < jnewbery> pinheadmz: yes
+13:21 < pinheadmz> all three, the same tx - not GETDATA(parent of tx)
+13:21 < jnewbery> (although of course we can't rely on the peer sending a NOTFOUND)
+13:22 < aj> INV(tx) GETDATA(tx) TX(txdata) is normal
+13:22 < pinheadmz> so is the tx not really in the maprealy from thebeginning? or is it actually dropped in the time between INV and GETDATA ?
+13:23 < aj> INV(tx) GETDATA(tx) TX(txdata) GETDATA(tx.parent) NOTFOUND is common though, because our side thinks "if they have tx, they must have the parent" and their side, running the same software, thinks "i didn't INV tx.parent, so i won't send it"
+13:23 < jnewbery> INV(tx_child) --->
+13:23 < jnewbery> <--- GETDATA(tx_child)
+13:23 < jnewbery> TX(tx_child) --->
+13:23 < jnewbery>                   I don't have this child's parents!
+13:23 < jnewbery> INV(tx_parent) --->
+13:23 < jnewbery> <--- GETDATA(tx_parent)
+13:23 < jnewbery> TX(tx_parent) --->
+13:23 < aj> jnewbery: the special logging mostly proved the guesses i was making was wrong, fwiw
+13:24 < jb55> I found reading the struct TxDownloadState comment helpful for anyone new to this part of the codebase
+13:24 < jnewbery> oh sorry, that second INV shouldn't be there
+13:24 < jnewbery> correction:
+13:24 < jnewbery> INV(tx_child) --->
+13:24 < jnewbery> <--- GETDATA(tx_child)
+13:24 < jnewbery> TX(tx_child) --->
+13:24 < jnewbery>                   I don't have this child's parents!
+13:24 < jnewbery> <--- GETDATA(tx_parent)
+13:24 < jnewbery> TX(tx_parent) --->
+13:25 < jnewbery> that last TX should actually be a NOTFOUND
+13:25 < pinheadmz> thanks! visual aids are so helpful :-)
+13:25 < pinheadmz> and why wouldnt the first node have the parent? bc it jsut expired? time-wise?
+13:25 < jnewbery> expired from the MapRelay but still in the mempool, or cofirmed in a block
+13:26 < Lightlike> but if it was confirmed in a block, wouldn't we know about this and not request it?
+13:27 < aj> so one thing i wonder is about sdaftuar's question, "switch the uses of GetTimeMicros() in the logic to GetTime() so that we can mock it"; it doesn't seem to make a lot of sense to measure timeouts in the seconds-minutes range with microseconds; but using non-mocktime for "peer has been idle" in general (see #9606) certainly makes sense i think
+13:27 < jnewbery> Lightlike: The peer might have it confirmed in a block, but we don't have the block yet? Not sure
+13:28 < aj> Lightlike: there's a bug in the handling of missing parents. when actually testing the transaction the logic is done right (in AcceptToMempool), but when that fails, it doesn't say what tx's exactly are missing
+13:29 < b10c> jnewbery: and the "expired from the MapRelay but still in the mempool" was introduced in #14897?
+13:29 < aj> Lightlike: so when the net_processing code works that out to try to ask for the parents (which generally won't work anyway) it tests to see if the parent is in the mempool, or txid:0 or txid:1 are in the UTXO set
+13:29 < aj> Lightlike: but if the output we're looking for is txid:3 and txid:0 and txid:1 are spent, we'll get a false positive
+13:30 < jnewbery> b10c: no, I believe that's been normal behaviour since earlier than that
+13:30 < aj> i think the tx's in maprelay are kept around so even if the tx expires from the mempool (by being mined, or because the mempool is full) it's still able to be relayed for the 15m's since being INV announced
+13:31 < jnewbery> the problem introduced by 14897 is adding txs to the node's m_tx_in_flight and then there being edge-cases where they're never removed
+13:32 < b10c> jnewbery: ah I see
+13:33 < aj> so previously, it would've just been "we sent a GETDATA and got a NOTFOUND? not big deal, moving on". but now it means a data structure doesn't get cleared, and eventually a limit gets hit, and behaviour changes
+13:34 < jb55> p2p code is scary
+13:34 < jnewbery> jb55: yes!
+13:34 < jb55> so much state
+13:34 < jonatack> can you expand on the missing testing and mocktime?
+13:35 < jnewbery> we talk a lot about consensus code being subtle and difficult, but problems in p2p code can be just as dangerous
+13:35  * jb55 considering spending more time looking at p2p PRs
+13:36 < jnewbery> luckily, I think there are more parallels between p2p code and other software domains (eg networking, telecoms), so for people with experience in those areas then p2p might be somewhere you want to look at more.
+13:37 < jonatack> * jb55: there are todos in https://github.com/bitcoin/bitcoin/pull/9606/files
+13:37 < jnewbery> jonatack: there are various calls for getting the current time used by bitcoind. GetTime() can be overridden by setmocktime but GetTimeMicros() always uses your system time
+13:38 < jnewbery> I *think* that's just historic. I can't think of any good reason for it
+13:38 < jnewbery> but it does mean that code that uses GetTimeMicros() can't be tested using mocktime
+13:39 < aj> jnewbery: i think if you used mocktime for everything you might get a bug where "setup test, connect peers. mocktime += 1 hour. oh all the peers have been non-responsive for an hour, mass disconnect. tests fail"
+13:39 < aj> jnewbery: GetTimeMicros() is used in bunches of places for "here's how long this piece of code took to run" logs, which is pretty reasonable too
+13:39 < jonatack> thanks! wdyt about sdaftuar's last comment/question
+13:40 < jonatack> could this be tested in compacted mocktime
+13:41 < Lightlike> A question concerning the fix in #15834: Now we clear m_tx_inflight_it after our peer first sends an INV(tx), but is totally unresponsive after GETDATA(tx) (not even NOTFOUND). At the same time we accept other transactions from this peer. is there a legit reason for our peer to behave like this besides censoring certain transactions (so that we don't disconnect instead)?
+13:42 < aj> i think it'd make sense to switch it to mockable GetTime,interested in what other people think, or if anyone sees any problems with that
+13:42 < jnewbery> on the face of it, it seems reasonable. I can't immediately think of any reason that it'd be a problem
+13:44 < aj> Lightlike: legit reasons: could be buggy, or maybe there's bugs in our logic where we're taking much longer to send the GETDATA than we think and it's actually totally reasonable for it to have expired, could be others
+13:44 < jnewbery> Lightlike: unreliable connection?
+13:45 < aj> jnewbery: unreliable with the bounds TCP/tor provides?
+13:45 < jnewbery> In general, I think we want to be careful about introducing behaviour that could turn a relatively small bug into something that causes a bunch of peers to disconnect
+13:45 < aj> yeah, that^^^
+13:47 < Lightlike> yes, that makes sense :-)
+13:49 < jnewbery> aj: yeah TCP/tor should give us reliable messages
+13:49 < jnewbery> jonatack: I hope it can be tested with mocktime, or in some way that doesn't require very long-running tests. The problem with long-running tests is that no-one ever runs them
+13:50 < jnewbery> (and we explicitly disable them from our Travis CI because they'd cause the run to time out)
+13:50 < jonatack> jnewbery: this
+13:51 < jnewbery> A few other random thoughts I had about this PR: I liked aj's update to the getpeerinfo RPC here: https://github.com/bitcoin/bitcoin/pull/15776/files#diff-618d3a4f4fc0dc851ec29218c445c148R109
+13:52 < jnewbery> Because I like monitoring in general
+13:53 < jnewbery> I'd also love to see more work on stuff like https://forkmonitor.info/nodes/btc to provide live monitoring of the network
+13:53 < jnewbery> I know that james o'beirne is planning to do some work on that, so if it's something that interests anyone here, please reach out to him
+13:54 < jnewbery> Another thing that I'd like to see is more stress testing. Resource leaks like this are really difficult to catch in deterministic, short-running tests
+13:55 < jnewbery> Any final questions?
+13:55 < aj> https://github.com/ajtowns/bitcoin/commits/201904-reduce-notfounds  -- is a branch i was playing with to reduce the underlying cause for getting notfounds btw. i haven't opened a PR for it yet, but it might be interesting to look at
+13:55 < jnewbery> aj: cool, thanks!
+13:56 < jnewbery> ok, seems like there aren't any more questions
+13:56 < jnewbery> This was quite a step up from the previous two weeks in terms of complexity - I'd be interested in what people thought about reviewing.
+13:57 < jnewbery> Net Processing is a tricky area. Probably second only to Validation in terms of subtlety and complexity. It'd be great if we had more eyes on it.
+13:58 < jnewbery> ok, thanks everyone, and thanks especially to aj!
+13:58 < jonatack> Good choice of PR -- I'm learning with these forays into new (for me) places in the codebase
+13:58 < Lightlike> I found this very interesting (the p2p code was new to me), but also definitely harder to understand than the one last week. Thanks!
+13:58 < merehap> jnewbery, aj: Thanks!
+13:58 < jonatack> Thanks aj and jnewbery!
+13:59 < aj> the p2p code was new to me too when trying to debug this :)
+13:59 < jnewbery> let's wrap it up there. As always, feel free to ping me if you have any thoughts about these.
+13:59 < pinheadmz> jnewbery: thank you! this is great, informative, fun
+13:59 < pinheadmz> to the moon!
+
+ + From 2dc9649250170003a3b1b28cff3c3c423f9579b3 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Tue, 13 Jun 2023 13:53:44 +0300 Subject: [PATCH 15/21] Add styles for meeting lists --- .../.vuepress/components/ReviewClubLayout.vue | 22 ++++++++++++++++++- package-lock.json | 4 ++-- package.json | 4 ++-- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/.vuepress/components/ReviewClubLayout.vue b/docs/.vuepress/components/ReviewClubLayout.vue index 8667dd03e2..71604cbe2a 100644 --- a/docs/.vuepress/components/ReviewClubLayout.vue +++ b/docs/.vuepress/components/ReviewClubLayout.vue @@ -20,7 +20,7 @@ export default { }, components: { ReviewClubHeader - } + }, } @@ -61,4 +61,24 @@ export default { .body-container a { text-decoration: underline; } + +.meeting-list-container .meeting-list { + list-style-type: none; +} + +#meeting-link { + color: #0000f0; +} + +#meeting-link:hover { + color: #fcc603; +} + +#meeting-link:visited { + color: #551b8c; +} + +#text-separator { + font-weight: bold; +} \ No newline at end of file diff --git a/package-lock.json b/package-lock.json index c6074e594e..322da91608 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,12 +11,12 @@ "devDependencies": { "@spiralbtc/vuepress-devkit-theme": "^0.5.0", "broken-link-checker": "0.7.8", + "he": "^1.2.0", "serve": "14.2.0", "start-server-and-test": "2.0.0", "vue": "2.7.14", "vue-server-renderer": "2.7.14", - "vuepress": "1.9.9", - "he": "^1.2.0" + "vuepress": "1.9.9" } }, "node_modules/@ampproject/remapping": { diff --git a/package.json b/package.json index 2e7e7648d3..8fd84e45c7 100644 --- a/package.json +++ b/package.json @@ -31,11 +31,11 @@ "devDependencies": { "@spiralbtc/vuepress-devkit-theme": "^0.5.0", "broken-link-checker": "0.7.8", + "he": "^1.2.0", "serve": "14.2.0", "start-server-and-test": "2.0.0", "vue": "2.7.14", "vue-server-renderer": "2.7.14", - "vuepress": "1.9.9", - "he": "^1.2.0" + "vuepress": "1.9.9" } } From 2a504d0ecde178e1aa234c2cd4ff7cff38f3d0d1 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 14 Jun 2023 12:22:28 +0300 Subject: [PATCH 16/21] Add irc meeting place --- docs/.vuepress/enhanceApp.js | 2 +- docs/review-club/README.md | 2 +- docs/review-club/your-first-meeting.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/.vuepress/enhanceApp.js b/docs/.vuepress/enhanceApp.js index 16f51bba6c..d433d756c6 100644 --- a/docs/.vuepress/enhanceApp.js +++ b/docs/.vuepress/enhanceApp.js @@ -10,7 +10,7 @@ export default ({ Vue }) => { created() { this.$meetingTime = "15:00 UTC"; this.$meetingDay = "Thursday"; - this.$meetingPlace = ""; + this.$meetingPlace = 'https://web.libera.chat/#bdk-pr-reviews'; }, }; diff --git a/docs/review-club/README.md b/docs/review-club/README.md index b68b48d4f1..2af1e82fd8 100644 --- a/docs/review-club/README.md +++ b/docs/review-club/README.md @@ -6,7 +6,7 @@ title: BDK Weekly PR review club ### A fortnightly review club for BitcoinDevKit(BDK) PRs What is this?  A fortnightly review club for BitcoinDevKit (BDK) PRs at **{{ $meetingTime }} on {{ $meetingDay }}s** in the #bdk-pr-reviews IRC channel on -{{ $meetingPlace }}. +Libera.chat. What's it for?  To help newer contributors learn about the BDK review process. The review club is *not* primarily diff --git a/docs/review-club/your-first-meeting.md b/docs/review-club/your-first-meeting.md index de7ff81229..69037788d5 100644 --- a/docs/review-club/your-first-meeting.md +++ b/docs/review-club/your-first-meeting.md @@ -9,7 +9,7 @@ If you’re thinking about attending PR Review Club, welcome! We love new participants. Below is information to help you get started as a first-timer. The club meets on {{ $meetingDay }}s at {{ $meetingTime }} in -{{ $meetingPlace }}. Every other week, a BDK +Libera.chat. Every other week, a BDK developer will host a 60-minute discussion on a BDK pull request. [The code of conduct](/review-club/code-of-conduct) details the behavior we expect from all participants. From 5c38801972994bd6278384db875c46a0b46829d2 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 14 Jun 2023 13:46:50 +0300 Subject: [PATCH 17/21] Scope CSS in Vue components --- docs/.vuepress/components/IRCWRAPPER.vue | 2 +- docs/.vuepress/components/ReviewClubHeader.vue | 2 +- docs/.vuepress/components/ReviewClubLayout.vue | 2 +- docs/.vuepress/components/ReviewClubMeetingLayout.vue | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/.vuepress/components/IRCWRAPPER.vue b/docs/.vuepress/components/IRCWRAPPER.vue index 132c27a755..ab7ccd76e7 100644 --- a/docs/.vuepress/components/IRCWRAPPER.vue +++ b/docs/.vuepress/components/IRCWRAPPER.vue @@ -77,7 +77,7 @@ export default { }; -