IRC meeting summary for 2018-05-10

• View this week’s log on BotBot.me or MeetBot
• Meeting minutes by MeetBot

Topics discussed during this weekly meeting included pull requests the meeting participants would most like to see reviewed, potentially moving off of GitHub if pages continue not to load reliably, how to fix a performance regression, future designs for storing transaction script data in memory, and whether or not create additional review queues.

High priority for review

Background: each meeting, Bitcoin Core developers discuss which Pull Requests (PRs) the meeting participants think most need review in the upcoming week. Some of these PRs are related to code that contributors especially want to see in the next release; others are PRs that are blocking further work or which require significant maintenance (rebasing) to keep in a pending state. Any capable reviewers are encouraged to visit the project’s list of current high-priority PRs.

Discussion (log): PRs specifically discussed included,

• [wallet] loadwallet RPC - load wallet at runtime (#10740). This is one of a set of PRs that (if accepted) will allow Bitcoin Core to create, load, and unload wallets at runtime in the context of the multiwallet mode added in Bitcoin Core 0.15.0. In the meeting, Joao Barbosa suggested this PR “is good to go” and Wladimir van der Laan said it “should be pretty close to mergeable.”

• BIP 158: Compact Block Filters for Light Clients (#12254). This PR allows Bitcoin Core to generate compact indices for some of the information contained within a block. These indices can then be shared with lightweight clients to allow them to determine whether or not the block contains information relevant to the client’s wallet, at which point the client can request to download the whole block (possibly from a different peer).

While discussing specific PRs, the issue of GitHub pages not loading (displaying a unicorn-themed error instead) was mentioned for the nth time. Matt Corallo said, “if this unicorns thing persists we’re gonna have to move off GitHub. Mostly it works if you refresh enough or are logged out, but neither of those is a solution when the refresh count is about 10, [and] we can’t use a platform where half the contributors can’t load PRs.”

Wladimir van der Laan replied, “agree. GitHub is pretty much useless this way.” Other participants shared their techniques for working around the issue some of the time.

Conclusion: reviewers are encouraged to visit the project’s list of current high-priority PRs. No specific plan for leaving GitHub was discussed; it seems likely that meeting participants are hoping that GitHub will fix their system.

Cache witness hash in CTransaction

Background: when Bitcoin Core needs to store a transaction in memory, it uses the CTransaction data type. This contains enough information to construct a witness transaction identifier (wtxid, or witness hash) for the transaction. The CTransaction data type could be extended to contain a pre-computed (cached) copy of the witness hash.

Discussion (log): Marco Falke requested the topic, cited PR #13011, and introduced the subject: “The witness hash is used for all loose transactions, so caching it would speed up validation (e.g. [AcceptToMemoryPool] and compact block relay). Also, the witness hash is equal to the “normal hash” [txid] for transactions without a witness, so there is overhead for rescan/reindex [but it] is currently minimal (since there are not many transactions with witness[es]). The gains of caching the witness hash dwarf any overhead during rescan/reindex, [in my opinion]. And of course, we can just rework rescan in a future PR.”

Pieter Wuille provided the counterpoint: “[The] downside is that it makes the transactions larger [in memory], and hardcodes some validation specific logic into the transaction data structure (which for example also affects serving blocks from disks, etc).”

Matt Corallo was in favor of the proposal, “[The] upside is we rectify a significant performance regression.”

Wuille suggested separating the data types used for transactions so that the witness hash didn’t need to be generated and stored when it isn’t needed. Wladimir van der Laan seemed to agree, saying “let’s not make the code a mess [in order] to rush ahead.”

Also discussed was whether or not the proposed change should be backported to the next minor version of Bitcoin Core. Corallo was in favor of backporting, but Van der Laan, Cory Fields, and Alex Morcos were opposed (although perhaps not strongly opposed).

Conclusion: No explicit conclusion. PR #13011 was added to the list of current high-priority PRs during the discussion.

One big malloc

Background: In the C++ programming language Bitcoin Core is written in, the primary function for Memory ALLOCation is called malloc(). Currently, as described in PR #13062, memory is allocated for the scripts in transactions in a way that optimizes for caching scriptPubKeys but which has suboptimal performance in other cases. That PR works towards separating the storage of scripts in memory from the way they are accessed in the program, so that both storage and access (representation) can be optimized.

Discussion (log): Cory Fields requested the topic and introduced it: “[Pieter Wuille] and I have discussed this briefly: […] one malloc for script data per block. [That] got me wondering if it’d be worthwhile to change the P2P [network protocol message] format to be more agreeable with allocations (next time we’re changing something that is, not for this alone).”

Jonas Schnelli offered an example of what that’d look like, as confirmed by Fields, “Things like inv size comes before the actual inv data.”

Subsequent discussion focused not on Field’s original point but on the advantages and disadvantages of the method PR #13062 uses to store scripts in memory, called span. Arguing against span, at least “unless there is demonstrated use” of it, was Matt Corallo.

Arguing in favor of span were Fields and Wuille. Fields said, “[It] seems absolutely necessary to me if we’re ever going to untangle our subsystems.” Wuille agreed, saying, “exactly: it’s abstracting representation from processing.”

Corallo rebutted, “Why? If it’s just operating on a CScript, it should just operate on a CScript.” After some more discussion, Corallo said he understood the potential advantages but still wanted to see it being used in a mergeable PR before making the change, “Yes, I get it, and I like the option…when we have a user.”

Conclusion: no explicit conclusion. Wuille and Fields may need to put more effort into proving the advantages of their approach before PRs related to it are accepted.

Review coordination

Background: the Bitcoin Core project currently has over 250 open PRs, almost all of which require review (or additional review) before they can be considered for merging. Contributors have been saying for years that they wished the project had more people spending more time reviewing PRs.

Discussion: Jim Posen requested and introduced the topic, “[In addition to the list of high priority PRs], I think there’s space for another list of things that have been concept ACK’d for people to review so that everyone is reviewing different stuff and there’s some actual coordination.”

Pieter Wuille suggested the relatively new website BitcoinACKS.com, which Posen agreed “is great.” Wuille supported Posen’s idea, “Having a better overview on what is concept ACKs (and, similarly, encouraging people to concept ACK/NACK quickly) would be good.”

Wladimir van der Laan argued against the idea, saying that “Now everyone can have a PR on [the current high priority list] that’s blocking them, which should foster cooperation.” Matt Corallo agreed, “the million-papercut-PR review approach doesn’t get us anywhere as a project, [but] reviewing things on the high priority list does (at least to me).”

Conclusion: no explicit conclusion. Corallo personally concluded, “I don’t think we’re making any more progress this week than we have the last 10 times we’ve discussed this.”

Comic relief

<morcos> i'm back
<wumpus> welcome back!
  <sipa> hi back, i'm pieter
<morcos> oh man...

 <cfields> well another option is std::allocator magic,
           without having to switch to Span
  <wumpus> noooo
  <wumpus> no magic
 <cfields> wumpus: i can't argue with that, it looks like voodoo
    <sipa> damn cool voodoo.
    <sipa> but voodoo.
  <wumpus> c++ is already too much voodoo
    <sipa> let's switch to BASIC

    <sipa> bitcoinacks.com ? :)
<BlueMatt> apparently it doesnt distinguish between nacks and acks
    <sipa> ha.
  <wumpus> lol that's an interesting bug
    <sipa> "nack" "nack" "nack" "merged!"

Participants

Disclaimer

This summary was compiled without input from any of the participants in the discussion, so any errors are the fault of the summary author and not the discussion participants. In particular, quotes taken from the discussion had their capitalization, punctuation, and spelling modified to produce consistent sentences. Bracketed words and fragments, as well as background narratives and exposition, were added by the author of this summary and may have accidentally changed the meaning of some sentences. If you believe any quote was taken out of context, please open an issue and we will correct the mistake.