IRC meeting summary for 2018-05-03

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

Topics discussed during this weekly meeting included what pull requests members of the project would like reviewers to focus on during the upcoming week, whether debug logging should be moved to a separate thread so that it doesn’t slow down certain use cases, whether to produce a 0.16.1 minor release containing some bugfixes and standard transaction policy changes, and how to allow messages received from other peers on the network to be processed in parallel.

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): Specific PRs discussed during the meeting included,

1. BIP 158: Compact Block Filters for Light Clients (#12254), nominated for inclusion on the list by Jim Posen. 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).

2. [wallet] loadwallet RPC - load wallet at runtime (#10740), nominated for inclusion on the list by John Newbery. 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.

3. UI: Support wallets loaded dynamically (#13097), nominated for inclusion on the list by Joao Barbosa. Built on top of the previously-mentioned PR #10740, this provides support in the Bitcoin Core Graphical User Interface (GUI) for dynamic loading of wallets.

The issue of some GitHub pages not loading (“unicorns”) was briefly mentioned in this meeting again for at least the fourth week running.

Delete 0.8, 0.9, and 0.10 git branches

Background: new development of Bitcoin Core generally occurs on the master branch of the Git repository. To create a stable release, the master branch is git-forked into a stable branch with the name of the intended release, e.g. for version 0.8 the branch is 0.8. The code on this branch is tested, matured, and released—and subsequent bug fixes for minor version releases (e.g. 0.8.1) are also made on this branch.

Discussion (log): Marco Falke proposed the topic shortly before the meeting and introduced it saying, “Those last tagged versions on those branches are EOL [End Of Life] for more than a year now. The tags can be kept for archival reasons, but the branches are no longer required. See https://bitcoincore.org/en/lifecycle/#schedule.”

Everyone who commented on the matter in the meeting approved. Luke Dashjr suggested that if the final commit on each branch wasn’t attached to a release tag (which indicates exactly which code was used for a particular release) that a tag be added to ensure anyone who needs that specific code can still obtain it. Other meeting participants agreed.

Conclusion: the branches were deleted shortly after the end of the meeting.

Moving logging to a separate thread

Background: by default, Bitcoin Core writes certain information about what it’s doing to a log file in case something goes wrong and the user needs to figure out what triggered the problem. Currently, this logging is done as a sequential step in the program’s execution so the next step after logging isn’t taken until the logging completes, this is described as the logging step “blocking” subsequent steps. Threading is a way for a program to tell the operating system that multiple steps can be executed in parallel, which could allow the next step in the program to start before logging completes (described as “non-blocking”).

Discussion (log): James O’Beirne suggested the topic and introduced it by saying, “I think it may be worthwhile to move logging into a separate thread.” He referenced two recent PRs related to logging, #13099 and #12970.

Matt Corallo was enthusiastic about the idea, saying “ACKACKACKACKACKACKACKACKACKACK” and pointing out that, “this is a surprisingly high lag-creator for miners, at least for those with spinning-disk-backed-or-cloud-hosted machines.” He also linked to a commit for his project Bitcoin FIBRE (a software fork of Bitcoin Core not used for consensus enforcement) that implements basic threading for logging.

Several other developers supported the idea and discussion focused around the best way to implement the behavior and, in particular, how to ensure that if something does go wrong, as much information as possible is still written to the log.

Some participants also discussed how much faster Bitcoin Core would be if logging was moved to a separate thread. General opinion seemed to be that it could help in some time-critical applications, such as miners announcing newly-discovered blocks, and would also help when users were running with optional verbose logging enabled for debugging. The later is something often done by developers and which is used automatically by parts of Bitcoin Core’s test suite. For other use cases, however, it was not expected to provide a significant improvement.

Conclusion: James O’Beirne finished the discussion by saying, “unless anyone has any objections, I’ll start working on a thread-consumes-from-ring-buffer implementation in the near future.”

0.16.1

Background: the most recent major version of Bitcoin Core, version 0.16.0, was released about two months ago. Often after a release, bugs affecting that release are fixed and certain new features are considered to be important enough to backport to that release, resulting in a new minor release.

Discussion (log): Matt Corallo request the topic and introduced it, “for those who weren’t paying attention, [Jesse Cohen] found some particularly novel races in block handling in #13092. Because they’re threading issues, they almost certainly won’t effect anyone except submitblock users, i.e. miners, and only [in] rare races [cases?] but, still, I think given that and some of the other various fixes we’ve had, it may be worth backporting.”

The cited issue, #13092, is an analysis by Suhas Daftuar of an issue uncovered by integration tests Cohen wrote in PR #13023. In the worst case, a miner could think that they’ve send a newly-found block to the network using the submitblock RPC only to discover that Bitcoin Core silently ignored the block because of a race condition, a situation where the program executed steps in a different order than the programmer expected. Cohen’s tests uncover this issue because they create several test blocks (with no proof of work) in a very short period of time. There are almost always larger gaps between blocks on the main network, so hopefully no miners have been affected by this bug so far.

Although there’s a PR to fix the issue, Daftuar believes additional discussion is needed in order “to settle on the right fix.” Cohen suggested #12988 is a “similar type of bug” and should possibly be fixed in the minor release as well.

Corallo also suggested that 0.16.1 should include PR #11423 by Johnson Lau to make the CodeSeparator opcode non-standard in spends of legacy (non-segwit) inputs. Non-standard means that nodes will not accept transactions with those inputs into the mempool; they will still accept them if they occur in a block. This should eliminate the use of a function called FindAndDelete() that has had a problematic history (see 1, 2). Segwit was implemented without a need for FindAndDelete but still provides the CodeSeparator opcode, which NBitcoin developer Nicholas Dorier has been using as part of a Tumblebit implementation—so making CodeSeparator non-standard in segwit spends has not been formally proposed.

There was some discussion about whether Lau’s PR was ready for merge. It was Corallo’s belief that Lau “wanted to add one more policy rule [to Lau’s PR].” Corallo said he’ll contact Lau about it.

Conclusion: all participants seemed in favor of putting together a 0.16.1 release to fix the race condition and include the additional standardness rule. During and subsequent to the meeting, the various PRs discussed were added to the project’s 0.16.1 milestone.

Call ProcessNewBlock asynchronously

Background: Bitcoin Core currently processes messages it receives from its peers on the peer-to-peer network in a single thread. If it could be rewritten to use multiple threads, it could process some messages in parallel, which could provide a performance improvement. However, because it often receives the same basic message from multiple peers, this presents a challenge of how to avoid doing duplicate work.

Discussion (log): Matt Corallo requested the topic and introduced it, “PR #12934 [is] certainly not ready for review, but we should maybe have a discussion about what concurrency across peers is gonna look like. There are two main approaches, but both end up requiring similar refactors for the majority of their work. In the past I’ve looked at doing ProcessMessages() in parallel; in [the previously-linked PR, Jesse Cohen] moves the validation processing of [transactions and blocks] into a queue and does that in a separate thread. In both cases, we end up building logic to ‘pause’ processing of a peer until whatever message it just generated has been processed.”

Gregory Maxwell and Corallo briefly discussed which parts of the system would find concurrency especially beneficial. For Maxwell, it was Initial Block Download (IBD) where download of new blocks is delayed “when connecting multiple blocks at a time (due to out of order fetching in IBD)”. For Corallo, it would be relaying gettxn (get transaction) requests during connection of a new block received using CompactBlocks (BIP152), “the one big cheapish win left for block-relay-latency [improvements].”

Corallo then pointed out that concurrency would allow newly-received data from multiple peers to be simultaneously deserialized into usable data structures on systems with multiple CPU cores, which Maxwell agreed could be a nice gain. They also agreed that the many improvements made to the code in the past year make it simpler and easier to implement this type of change.

Conclusion: Cohen wrote, “Cool—so, if no strong objections or concerns with the approach, I’ll continue this and come back when it’s more ready for review.”

Comic relief

       <wumpus>  #10740 given me an unicorn though
<jonasschnelli>  has also a unicorn, reload solved
       <wumpus>  yes
       <LukeJr>  unicorns probably have a high street value
     <jnewbery>  not any more. The market's been flooded
       <LukeJr>  shows what I know of unicorn markets
       <wumpus>  LukeJr: yes, I"m trying to farm them and sell them,
                 but I have more now than atoms in the knows universe
                 so you could say the supply is more than the demand...

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 contact us and we will correct the mistake.