Skip to content

Commit 520d745

Browse files
hardingharding
committed
Merge #553: Meetings: add 2018-05-10
bbec1dc Meetings: add 2018-05-10 (David A. Harding) Pull request description: Preview: http://dg2.dtrt.org/en/meetings/2018/05/10/ Tree-SHA512: f78e3afca73677fc6e91fe60d2288af814f0d9eda7335718f62505a7ddb8daacb0b926430396f2bd051e1d5cd06e88e863b8e32dc93401cfec86f69faecf1ba0
2 parents eb9ac95 + bbec1dc commit 520d745

File tree

2 files changed

+255
-0
lines changed

2 files changed

+255
-0
lines changed

‎_includes/_references.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
[Russell Yanofsky]: https://github.com/ryanofsky
4848
[Saleem Rashid]: https://github.com/saleemrashid
4949
[Samuel Dobson]: https://github.com/MeshCollider
50+
[Sjors Provoost]: https://github.com/sjors
5051
[Suhas Daftuar]: https://github.com/sdaftuar
5152
[Tadge Dryja]: https://github.com/T909
5253
[Tom Harding]: https://github.com/dgenr8

‎_posts/en/meetings/2018-05-10-meeting.md

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
---
2+
title: IRC meeting summary for 2018-05-10
3+
permalink: /en/meetings/2018/05/10/
4+
name: 2018-05-10-meeting
5+
type: meetings
6+
layout: page
7+
lang: en
8+
version: 1
9+
---
10+
{% include _toc.html %}
11+
{% include _references.md %}
12+
13+
- View this week's log on [BotBot.me](https://botbot.me/freenode/bitcoin-core-dev/msg/99928845/) or [MeetBot](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-10-19.00.log.html)
14+
- [Meeting minutes by MeetBot](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-10-19.00.html)
15+
16+
---
17+
18+
Topics discussed during this weekly meeting included pull requests the
19+
meeting participants would most like to see reviewed, potentially moving
20+
off of GitHub if pages continue not to load reliably, how to fix a
21+
performance regression, future designs for storing transaction script
22+
data in memory, and whether or not create additional review queues.
23+
24+
## High priority for review
25+
26+
**Background:** each meeting, Bitcoin Core developers discuss which Pull
27+
Requests (PRs) the meeting participants think most need review in the
28+
upcoming week. Some of these PRs are related to code that contributors
29+
especially want to see in the next release; others are PRs that are
30+
blocking further work or which require significant maintenance (rebasing)
31+
to keep in a pending state. Any capable reviewers are encouraged to
32+
visit the project's list of [current high-priority
33+
PRs][].
34+
35+
**Discussion
36+
([log](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-10-19.00.log.html#l-16)):** PRs specifically discussed included,
37+
38+
- **[wallet] `loadwallet` RPC - load wallet at runtime ([#10740][]).**
39+
This is one of a set of PRs that (if accepted) will allow Bitcoin Core
40+
to create, load, and unload wallets at runtime in the context of the
41+
multiwallet mode added in Bitcoin Core 0.15.0. In the meeting, Joao
42+
Barbosa suggested this PR "is good to go" and Wladimir van der Laan
43+
said it "should be pretty close to mergeable."
44+
45+
- **BIP 158: Compact Block Filters for Light Clients ([#12254][]).**
46+
This PR allows Bitcoin Core to generate compact indices for some of
47+
the information contained within a block. These indices can then be
48+
shared with lightweight clients to allow them to determine whether or
49+
not the block contains information relevant to the client's wallet, at
50+
which point the client can request to download the whole block
51+
(possibly from a different peer).
52+
53+
While discussing specific PRs, the issue of GitHub pages not loading
54+
(displaying a unicorn-themed error instead) was mentioned for the nth
55+
time. Matt Corallo said, "if this unicorns thing persists we're gonna
56+
have to move off GitHub. Mostly it works if you refresh enough or are
57+
logged out, but neither of those is a solution when the refresh count is
58+
about 10, [and] we can't use a platform where half the contributors
59+
can't load PRs."
60+
61+
Wladimir van der Laan replied, "agree. GitHub is pretty much useless
62+
this way." Other participants shared their techniques for working
63+
around the issue some of the time.
64+
65+
**Conclusion:** reviewers are encouraged to visit the project's list of
66+
[current high-priority PRs][]. No specific plan for leaving GitHub was
67+
discussed; it seems likely that meeting participants are hoping that
68+
GitHub will fix their system.
69+
70+
## Cache witness hash in CTransaction
71+
72+
**Background:** when Bitcoin Core needs to store a transaction in
73+
memory, it uses the `CTransaction` data type. This contains enough
74+
information to construct a witness transaction identifier (*wtxid*, or
75+
*witness hash*) for the transaction. The CTransaction data type could
76+
be extended to contain a pre-computed (cached) copy of the witness hash.
77+
78+
**Discussion
79+
([log](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-10-19.00.log.html#l-51)):**
80+
Marco Falke requested the topic, cited PR [#13011][], and introduced the
81+
subject: "The witness hash is used for all loose transactions, so
82+
caching it would speed up validation (e.g. [AcceptToMemoryPool] and
83+
compact block relay). Also, the witness hash is equal to the "normal
84+
hash" [txid] for transactions without a witness, so there is overhead
85+
for rescan/reindex [but it] is currently minimal (since there are not
86+
many transactions with witness[es]). The gains of caching the witness
87+
hash dwarf any overhead during rescan/reindex, [in my opinion]. And of
88+
course, we can just rework rescan in a future PR."
89+
90+
Pieter Wuille provided the counterpoint: "[The] downside is that it
91+
makes the transactions larger [in memory], and hardcodes some validation
92+
specific logic into the transaction data structure (which for example
93+
also affects serving blocks from disks, etc)."
94+
95+
Matt Corallo was in favor of the proposal, "[The] upside is we rectify a
96+
significant performance regression."
97+
98+
Wuille suggested separating the data types used for transactions so that
99+
the witness hash didn't need to be generated and stored when it isn't
100+
needed. Wladimir van der Laan seemed to agree, saying "let's not make
101+
the code a mess [in order] to rush ahead."
102+
103+
Also discussed was whether or not the proposed change should be
104+
backported to the next minor version of Bitcoin Core. Corallo was in
105+
favor of backporting, but Van der Laan, Cory Fields, and Alex Morcos
106+
were opposed (although perhaps not strongly opposed).
107+
108+
**Conclusion:** No explicit conclusion. PR [#13011][] was added to the
109+
list of [current high-priority PRs][] during the discussion.
110+
111+
## One big malloc
112+
113+
**Background:** In the C++ programming language Bitcoin Core is written
114+
in, the primary function for Memory ALLOCation is called `malloc()`. Currently,
115+
as described in PR [#13062][], memory is allocated for the scripts
116+
in transactions in a way that optimizes for caching scriptPubKeys but
117+
which has suboptimal performance in other cases. That PR works towards
118+
separating the storage of scripts in memory from the way they are
119+
accessed in the program, so that both storage and access
120+
(representation) can be optimized.
121+
122+
**Discussion
123+
([log](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-10-19.00.log.html#l-109)):**
124+
Cory Fields requested the topic and introduced it: "\[Pieter Wuille] and
125+
I have discussed this briefly: [...] one malloc for script data per
126+
block. [That] got me wondering if it'd be worthwhile to change the P2P
127+
[network protocol message] format to be more agreeable with allocations
128+
(next time we're changing something that is, not for this alone)."
129+
130+
Jonas Schnelli offered an example of what that'd look like, as confirmed
131+
by Fields, "Things like `inv` size comes before the actual `inv` data."
132+
133+
Subsequent discussion focused not on Field's original point but on the
134+
advantages and disadvantages of the method PR [#13062][] uses to store
135+
scripts in memory, called `span`. Arguing against `span`, at least
136+
"unless there is demonstrated use" of it, was Matt Corallo.
137+
138+
Arguing in favor of `span` were Fields and Wuille. Fields said, "[It]
139+
seems absolutely necessary to me if we're ever going to untangle our
140+
subsystems." Wuille agreed, saying, "exactly: it's abstracting
141+
representation from processing."
142+
143+
Corallo rebutted, "Why? If it's just operating on a CScript, it should
144+
just operate on a CScript." After some more discussion, Corallo said he
145+
understood the potential advantages but still wanted to see it being
146+
used in a mergeable PR before making the change, "Yes, I get it, and I
147+
like the option...when we have a user."
148+
149+
**Conclusion:** no explicit conclusion. Wuille and Fields may need to
150+
put more effort into proving the advantages of their approach before PRs
151+
related to it are accepted.
152+
153+
## Review coordination
154+
155+
**Background:** the Bitcoin Core project currently has over 250 open
156+
PRs, almost all of which require review (or additional review) before
157+
they can be considered for merging. Contributors have been saying for
158+
years that they wished the project had more people spending more time
159+
reviewing PRs.
160+
161+
**Discussion:** Jim Posen requested and introduced the topic, "[In
162+
addition to the list of high priority PRs], I think there's space for
163+
another list of things that have been concept ACK'd for people to review
164+
so that everyone is reviewing different stuff and there's some actual
165+
coordination."
166+
167+
Pieter Wuille suggested the relatively new website
168+
[BitcoinACKS.com](https://bitcoinacks.com/), which Posen agreed "is
169+
great." Wuille supported Posen's idea, "Having a better overview on
170+
what is concept ACKs (and, similarly, encouraging people to concept
171+
ACK/NACK quickly) would be good."
172+
173+
Wladimir van der Laan argued against the idea, saying that "Now everyone
174+
can have a PR on [the current high priority list] that's blocking them,
175+
which should foster cooperation." Matt Corallo agreed, "the
176+
million-papercut-PR review approach doesn't get us anywhere as a
177+
project, [but] reviewing things on the high priority list does (at least
178+
to me)."
179+
180+
**Conclusion:** no explicit conclusion. Corallo personally concluded,
181+
"I don't think we're making any more progress this week than we have the
182+
last 10 times we've discussed this."
183+
184+
## Comic relief
185+
186+
{% highlight text %}
187+
<morcos> i'm back
188+
<wumpus> welcome back!
189+
<sipa> hi back, i'm pieter
190+
<morcos> oh man...
191+
{% endhighlight %}
192+
193+
{% highlight text %}
194+
<cfields> well another option is std::allocator magic,
195+
without having to switch to Span
196+
<wumpus> noooo
197+
<wumpus> no magic
198+
<cfields> wumpus: i can't argue with that, it looks like voodoo
199+
<sipa> damn cool voodoo.
200+
<sipa> but voodoo.
201+
<wumpus> c++ is already too much voodoo
202+
<sipa> let's switch to BASIC
203+
{% endhighlight %}
204+
205+
{% highlight text %}
206+
<sipa> bitcoinacks.com ? :)
207+
<BlueMatt> apparently it doesnt distinguish between nacks and acks
208+
<sipa> ha.
209+
<wumpus> lol that's an interesting bug
210+
<sipa> "nack" "nack" "nack" "merged!"
211+
{% endhighlight %}
212+
213+
## Participants
214+
215+
| IRC nick | Name/Nym |
216+
|-----------------|---------------------------|
217+
| wumpus | [Wladimir van der Laan][] |
218+
| BlueMatt | [Matt Corallo][] |
219+
| sipa | [Pieter Wuille][] |
220+
| jimpo | [Jim Posen][] |
221+
| cfields | [Cory Fields][] |
222+
| jonasschnelli | [Jonas Schnelli][] |
223+
| MarcoFalke | [Marco Falke][] |
224+
| promag | [Joao Barbosa][] |
225+
| luke-jr | [Luke Dashjr][] |
226+
| jamesob | [James O'Beirne][] |
227+
| morcos | [Alex Morcos][] |
228+
| achow101 | [Andrew Chow][] |
229+
| jcorgan | [Johnathan Corgan][] |
230+
| kanzure | [Bryan Bishop][] |
231+
| provoostenator | [Sjors Provoost][] |
232+
233+
234+
## Disclaimer
235+
236+
This summary was compiled without input from any of the participants in
237+
the discussion, so any errors are the fault of the summary author and
238+
not the discussion participants. In particular, quotes taken from the
239+
discussion had their capitalization, punctuation, and spelling modified
240+
to produce consistent sentences. Bracketed words and fragments, as well
241+
as background narratives and exposition, were added by the author of
242+
this summary and may have accidentally changed the meaning of some
243+
sentences. If you believe any quote was taken out of context, please
244+
[open an issue](https://github.com/bitcoin-core/bitcoincore.org/issues/new)
245+
and we will correct the mistake.
246+
247+
248+
249+
[#10740]: https://github.com/bitcoin/bitcoin/issues/10740
250+
[#12254]: https://github.com/bitcoin/bitcoin/issues/12254
251+
[#13011]: https://github.com/bitcoin/bitcoin/issues/13011
252+
[#13062]: https://github.com/bitcoin/bitcoin/issues/13062
253+
[current high-priority PRs]: https://github.com/bitcoin/bitcoin/projects/8
254+

0 commit comments

Comments
 (0)