IRC meeting summary for 2018-11-01

Overview

Topics covered during this weeks contributor meeting included pull requests nominated for high priority review, the absence of tests for non-HD wallet code paths, a review of the progress made on refactoring the wallet and the spurious failures produced by tests run on AppVeyor CI. To complete the meeting, contributors were encouraged to share the projects they are actively working on.

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): the following PRs were discussed:

  • #14532 - Never bind INADDR_ANY by default, and warn when doing so explicitly by luke-jr

  • #14350 - Add WalletLocation class by promag

  • #14046 - net: Refactor message parsing (CNetMessage) by jonasschnelli

  • #14477 - Add ability to convert solvability info to descriptor by sipa

  • #13932 - Additional utility RPCs for PSBT by achow101

  • #14437 - Refactor: Start to separate wallet from node by ryanofsky

  • #14336 - net: implement poll by pstratem

non-HD Wallet Tests

Background: Bitcoin Core introduced support for BIP32 Hierarchical Deterministic wallets in 0.13.0, greatly simplifying the wallet backup and restoration processes. Instead of making regular backups of an amorphous keypool, a single backup can be made of the wallet’s extended private masterkey (from which all keys in the wallet are deterministically generated). Prior to version 0.16.0, users were given the option of disabling this feature by specifying -usehd=0. When this option was removed, so were the tests for non-HD wallet code paths. While non-HD wallets can no longer be created by newer versions of Bitcoin Core, they can still be imported.

Discussion (log): This topic was suggested by luke-jr. Because Bitcoin Core no longer creates non-HD wallets, it was mentioned by provoostenator, achow101 and jfnewbery that non-HD versions of the wallet would have to be packaged into the test framework. Sipa pointed out the importance of testing wallet upgrade scenarios (the failure modes of a wallet that is being backed up and restored when Bitcoin Core is being upgraded and/or downgraded can lead to irrecoverable loss of funds).

Conclusion: Discussion of the various approaches should be continued in issue #14536.

Wallet Refactor

Background: The Bitcoin Core wallet is actively being refactored. This work has a number of goals, some of which include: improving upon the architectural modularity of Bitcoin Core, simplifying the wallet’s codebase and introducing new features. One project within this broader set of goals is the output descriptors language. Introduced in 0.17.0, this language is designed to encode the spending requirements of key sets into a single string; this approach to classifying unspent outputs improves upon the preexisting isMine logic, the complexities and inefficiencies of which impede the development of more advanced wallet features (e.g. hardware wallet integration that supports PSBT-compatible key signing protocols).

Discussion (log): In #14565, Sipa continues his work overhauling the logic of the importmulti RPC to restrict superfluous key or script data from being imported; this is necessary to support “old style” descriptor imports that implicitly convert the descriptor into existing wallet structures (#14491). Sipa will also tackle some pubkey-caching prep work required to use descriptors instead of keypools. To support native descriptor imports, the existing keypool/isMine logic must be refactored to sit behind an abstraction that can be instantiated by old wallet logic or descriptors.

Conclusion: Meshcollider will rebase #14491 when #14565 is complete. Achow101 will rebase #14075 on top of #14491 thereafter. #14705 enables a wallet with –disable-private-keys set to import and fetch pubkeys to/from the keypool. This aids in the “disentanglement” of watch-only wallets from wallets that store private keys. Without this PR, a wallet with disabled private keys will not fetch pubkeys from the keypool. This is useful for hardware wallets that interact with Bitcoin Core as an external signer; to support this use case, the keypool must accept the importation and retrieval of watch-only pubkeys generated by the hardware wallet. Discussion was concluded with the suggestion that a status update from ryanofsky on his code separation PRs be saved for the second Bitcoin Core Wallet Contributor meeting.

Note: During the “wallet stuff” session at the CoreDev meeting in Tokyo, it was proposed that a separate wallet-focused meeting be organized. On Friday October 19th, the first Bitcoin Core Wallet Meeting was held. It is scheduled for every other Friday at 19:00:00 UTC. The second meeting was held on November 2nd.

AppVeyor Failures

Background: AppVeyor is a hosted continuous integration service that provides Microsoft Visual C++ (MSVC) build environments to Bitcoin Core. Binaries produced in these environments are for cross-platform testing purposes only.

Discussion (log): The functional tests run on AppVeyor fail spuriously. This frustrates some of the contributors. Sipa requests that this issue be addressed.

Conclusion: There is an open issue #14446 which details the failure types. PR #13501 may mitigate one class of these failures.

What are people working on?

log

  • jarthur: UNIX domain sockets for the RPC API. Previous discussion. Open issue.
  • luke-jr: rebasing his PRs
  • achow101: PSBT and hardware wallet integration
  • jnewbery: spend more time reviewing wallet PRs by the end of the year
  • instagibbs: reviewing wallet PRs and hardware wallet integration
  • sipa: private authentication protocol for P2P network. Previous discussion. More reading (note: there are known vulnerabilities with the proposed protocol).
  • meshcollider: wallet refactor and review

Participants

IRC nick Name/Nym
sipa Pieter Wuille
provoostenator Sjors Provoost
luke-jr Luke Dashjr
achow101 Andrew Chow
MarcoFalke Marco Falke
meshcollider Samuel Dobson
jnewbery John Newbery
instagibbs Gregory Sanders
phantomcircuit Patrick Strateman
jarthur Justin Arthur
kanzure Bryan Bishop
gwillen Glenn Willen
ryanofsky Russell Yanofsky

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.