|
| 1 | +--- |
| 2 | +title: IRC meeting summary for 2018-04-12 |
| 3 | +permalink: /en/meetings/2018/04/12/ |
| 4 | +name: 2018-04-12-meeting |
| 5 | +type: meetings |
| 6 | +layout: page |
| 7 | +lang: en |
| 8 | +version: 1 |
| 9 | +--- |
| 10 | +{% include _toc.html %} |
| 11 | +{% include _references.md %} |
| 12 | + |
| 13 | +- [Link to this week logs](https://botbot.me/freenode/bitcoin-core-dev/2018-04-12/?msg=98918663&page=4) |
| 14 | +- [Meeting minutes by meetbot](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-04-12-19.01.html) |
| 15 | + |
| 16 | +--- |
| 17 | + |
| 18 | +Topics discussed during this weekly meeting included what pull requests |
| 19 | +members of the project would like reviewers to focus on during the |
| 20 | +upcoming week, whether or not to break compatibility with an odd and |
| 21 | +rarely used feature in the wallet (IsMine bare multisig), how to safely |
| 22 | +improve multiwallet support, and how to deal with some unsupported |
| 23 | +software when upgrading the build environment for Bitcoin Core's |
| 24 | +reproducible binary releases. |
| 25 | + |
| 26 | +## High priority for review |
| 27 | + |
| 28 | +*Background:* each meeting, Bitcoin Core developers discuss which Pull |
| 29 | +Requests (PRs) the meeting participants think most need review in the |
| 30 | +upcoming week. Some of these PRs are related to code that contributors |
| 31 | +especially want to see in the next release; others are PRs that are |
| 32 | +blocking further work or which require significant maintenance (rebasing) |
| 33 | +to keep in a pending state. Any capable reviewers are encouraged to |
| 34 | +visit the project's list of [current high-priority |
| 35 | +PRs](https://github.com/bitcoin/bitcoin/projects/8). |
| 36 | + |
| 37 | +*Discussion:* The following PRs were suggested for attention this week: |
| 38 | + |
| 39 | +1. **Build tx index in parallel with validation ([#11857][])** nominated |
| 40 | + by Jim Posen and Wladimir van der Laan. This PR moves the optional |
| 41 | + transaction index that allows users to look up historical |
| 42 | + transactions by their txid into a separate database. This will allow |
| 43 | + users to enable or disable the transaction index while a node is |
| 44 | + running instead of having to wait for up to several hours even on |
| 45 | + fast hardware. The PR also lays the groundwork for adding additional |
| 46 | + separate indices to Bitcoin Core that can enable features such as |
| 47 | + improved-privacy block filtering for lightweight clients as described |
| 48 | + in BIPs [157][BIP157] and [158][BIP158]. |
| 49 | + |
| 50 | + Prior to the meeting, the PR has already received some positive |
| 51 | + review but a few changes were requested and have now been made, so |
| 52 | + discussion in the meeting focused on getting those changes |
| 53 | + themselves reviewed. |
| 54 | + |
| 55 | +2. **Upgrade path for non-HD wallets to HD ([#12560][])** nominated by |
| 56 | + Van der Laan. This PR adds a new `sethdseed` (set HD seed) RPC that |
| 57 | + allows users to set the seed value for a [BIP32][] Hierarchical |
| 58 | + Deterministic (HD) wallet. This can be used to request Bitcoin Core |
| 59 | + generate a new seed on its own or to change the seed to a value |
| 60 | + obtained from outside the currently-running Bitcoin Core (such as |
| 61 | + from a backup). In addition, the PR allows users of older non-HD |
| 62 | + Bitcoin Core wallets to upgrade to HD wallets using the |
| 63 | + `-upgradewallet` command line parameter. |
| 64 | + |
| 65 | + Discussion in the meeting indicated that the PR may have one |
| 66 | + unaddressed issue but that the PR primarily needs review from |
| 67 | + additional contributors. |
| 68 | + |
| 69 | +3. **Move fee estimator into validationinterface/cscheduler thread |
| 70 | + ([#11775][])** nominated by Van der Laan. This PR makes a backend |
| 71 | + change to some internal interfaces in Bitcoin Core's code and then |
| 72 | + changes Bitcoin Core's fee estimator to use one of those interfaces, |
| 73 | + giving it access to additional information useful for fee estimation. |
| 74 | + The PR also makes some minor improvements to the fee estimator in the |
| 75 | + way that it handles edge cases. |
| 76 | + |
| 77 | + Prior to the meeting, the author of the PR (Matt Corallo) had |
| 78 | + offered to split the PR into separate smaller PRs for the different |
| 79 | + parts of the change. Discussion during the meeting indicated that |
| 80 | + reviewers are waiting for this to happen before reviewing further. |
| 81 | + |
| 82 | +4. **Introduce `getblockstats` RPC to [provide data that can be used to] |
| 83 | + plot things ([#10757][])** nominated by Jorge Timón. This PR adds a |
| 84 | + new RPC that returns various details and statistics about a specified |
| 85 | + block. |
| 86 | + |
| 87 | + Discussion in the meeting saw one contributor agreeing to review the |
| 88 | + PR. |
| 89 | + |
| 90 | +A concern mentioned during the discussion was GitHub's recent problem of |
| 91 | +pages failing to load for significant periods of time. The problem has |
| 92 | +become frequent enough that meeting participants mention the problem |
| 93 | +by reference to the drawing GitHub displays on the error page, a |
| 94 | +unicorn. |
| 95 | + |
| 96 | +## What to do with IsMine on bare multisig |
| 97 | + |
| 98 | +*Background:* Bare multsig refers to a payment to multiple public keys |
| 99 | +without using a much more commonly used P2SH or segwit address. Bitcoin |
| 100 | +Core's wallet currently scans any bare multisig payments to see if every |
| 101 | +public key used is part of the user's wallet and, if so, categorizes the |
| 102 | +payment as *InMine* to indicate that it's spendable by the user. |
| 103 | + |
| 104 | +*Discussion:* Pieter Wuille requested the topic. He described the |
| 105 | +current behavior as, "stupid, annoying, pointless, and hard to |
| 106 | +maintain." For *stupid* and *pointless,* the issue is probably that |
| 107 | +multisig doesn't provide any additional security over single-sig when |
| 108 | +all of the public keys belong to the same wallet, but using multisig |
| 109 | +does cost more than single-sig. For *annoying* and *hard to maintain,* |
| 110 | +the issue is that this behavior requires extra code and is making it more |
| 111 | +difficult for developers to work towards an [improved wallet |
| 112 | +design](https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2) |
| 113 | +previously described by Wuille. |
| 114 | + |
| 115 | +Wuille then described his concern: "there may be existing outputs to it. |
| 116 | +I don't know if this is the case or not, but it sounds scary to just |
| 117 | +stop supporting it." After some discussion, Wuille clarified that he |
| 118 | +isn't proposing to remove the code that allows owners of those payments |
| 119 | +to spend them; rather, it would be the case that Bitcoin Core would no |
| 120 | +longer automatically see those payments. |
| 121 | + |
| 122 | +Additional discussion between Matt Corallo and Wuille provided a vivid |
| 123 | +description of the problem: to support this old behavior in the desired |
| 124 | +new wallet model would require generating N<sup>3</sup> combinations, |
| 125 | +where *N* is the number of private keys the user owns. By default, |
| 126 | +Bitcoin Core generates 2,000 private keys, so the wallet would need to |
| 127 | +generate an unmanageable eight billion combinations. |
| 128 | + |
| 129 | +Continued discussion surrounded Wuille's PR [#12874][] which disables |
| 130 | +the current behavior and provides a workaround for users who need it. |
| 131 | +Corallo mentioned that the existing RPC `importaddress` already provides |
| 132 | +the important features necessary for the workaround. |
| 133 | + |
| 134 | +*Conclusion:* although meeting participants were in favor of preserving |
| 135 | +existing functionality if there was a reasonable way to do so, |
| 136 | +discussion seemed to favor removing IsMine on bare multisig and |
| 137 | +mentioning the workaround in the release notes. |
| 138 | + |
| 139 | +## Dynamic wallet load/unload |
| 140 | + |
| 141 | +*Background:* Old versions of Bitcoin Core could only use a single |
| 142 | +instance of the built in wallet with a particular node. This was |
| 143 | +extended to allow multiple wallet instances in [version |
| 144 | +0.15.0](/en/releases/0.15.0/#multi-wallet-support), but older parts of |
| 145 | +Bitcoin Core (such as command-line options read on startup) assume |
| 146 | +the user only has one wallet instance and so they act on all loaded |
| 147 | +wallets at once. John Newbery's pull request [#10740][] provides a |
| 148 | +workaround for this by allowing users to load, unload, and maybe even |
| 149 | +create wallets at run time using new proposed RPCs. |
| 150 | + |
| 151 | +*Discussion:* Joao Barbosa requested the topic, suggesting that |
| 152 | +"...wallet management should be with shared pointers". This would help |
| 153 | +ensure that if a user requests to unload a wallet at run time, pending |
| 154 | +requests involving that wallet can be handled before the wallet is |
| 155 | +unloaded. The alternative could be a wallet disappears unexpectedly in |
| 156 | +the middle of an operation, which could cause unexpected and harmful |
| 157 | +effects. Barbosa has opened PR [#11402][] to make this proposed |
| 158 | +change. |
| 159 | + |
| 160 | +This proposal seemed uncontroversial and discussion moved to other |
| 161 | +improvements and the sequence in which those improvements should be |
| 162 | +implemented. Jonas Schnelli suggested that Bitcoin Core first needed a |
| 163 | +`createwallet` RPC so that wallets could be created at run time and the |
| 164 | +command-line wallet options could be retired (or possibly restricted to |
| 165 | +just single-wallet use). Pieter Wuille noted that a runtime `createwallet` |
| 166 | +would be strange without a runtime way to load a wallet, as you'd have to |
| 167 | +restart Bitcoin Core in order to use the wallet you just created. |
| 168 | + |
| 169 | +Luke Dashjr suggested the sequence "load -> create -> unload" as "unload |
| 170 | +is the complex part," likely because of the potential problems dealing |
| 171 | +with multiple processes working on a loaded wallet simultaneously. |
| 172 | +Newbery concurred and responded favorably to Schnelli's suggestion |
| 173 | +to "split unload away from the existing PR." |
| 174 | + |
| 175 | +Wladimir van der Laan suggested that "unload should probably be in two |
| 176 | +stages: after requesting it, RPC and the GUI lose access to it. Then it |
| 177 | +waits for current operations to finish. Then the thing really gets |
| 178 | +[unloaded]." |
| 179 | + |
| 180 | +*Conclusion:* Newbery agreed that he'll modify his PR, saying on the PR |
| 181 | +itself that he'll "reduce the scope of this PR to just a loadwallet RPC. |
| 182 | +A createwallet RPC should come next, followed by unloadwallet. |
| 183 | +(unloadwallet is where most of the difficulties are)." |
| 184 | + |
| 185 | +## Gitian update |
| 186 | + |
| 187 | +*Background:* Bitcoin Core uses a system called [Gitian][] to build its |
| 188 | +release binaries in a way that is fully reproducible by anyone with a |
| 189 | +computer and Internet connection, allowing anyone to verify that the |
| 190 | +release binaries are the product of the peer-reviewed source code. As |
| 191 | +Bitcoin Core changes, the operating system targeted for building changes, |
| 192 | +and the availability of other software changes, the Gitian build |
| 193 | +environment needs to be updated to handle the changes. |
| 194 | + |
| 195 | +*Discussion:* Luke Dashjr requested the topic, which seemed to be |
| 196 | +related to the project's desire to switch the operating system used by |
| 197 | +Gitian to the upcoming Ubuntu 18.04 LTS. Dashjr's concern is that "we |
| 198 | +need a replacement for vmbuilder or something, since Canonical hasn't |
| 199 | +updated it to support anything recent." Vmbuilder is a tool that allows |
| 200 | +one to create and run a subsidiary operating system under your regular |
| 201 | +operating system in order to build software in it. A desirable feature |
| 202 | +of vmbuilder is that it can create the sub-operating system in a virtual |
| 203 | +machine to fully isolate it from your main operating system, helping to |
| 204 | +prevent any problems in the code you're building from affecting your |
| 205 | +actual operating system. |
| 206 | + |
| 207 | +Wladimir van der Laan suggested using debbootstrap (Debian bootstrap), a |
| 208 | +tool that predates vmbuilder and which uses a chroot instead of a |
| 209 | +virtual machine, allowing it to trick normal software into thinking it's |
| 210 | +being built in a separate operating system but which does nothing |
| 211 | +significant to prevent malicious software from attacking the primary |
| 212 | +operating system. Although other developers such as Cory Fields were in |
| 213 | +favor of moving to debootstrap, Dashjr remained concerned and said, "I |
| 214 | +suppose fixing vmbuilder might be not too unreasonable [an] effort, maybe I |
| 215 | +will try that." |
| 216 | + |
| 217 | +Andrew Chow added that he was "considering adding Docker support to Gitian |
| 218 | +so we would use a default Ubuntu docker image and then build from |
| 219 | +there." Docker can be more secure than a chroot but is usually less |
| 220 | +secure than a virtual machine. Dashjr noted that Docker also restricted to |
| 221 | +the x86_64 platform used by most desktops and servers, whereas some |
| 222 | +project contributor believe it would be advantageous for some of the |
| 223 | +reproducible builds to be created on other platforms that may not have |
| 224 | +the problems found on x86_64 such as those related to the [Intel |
| 225 | +Management Engine](https://en.wikipedia.org/wiki/Intel_Management_Engine). |
| 226 | + |
| 227 | +*Conclusion:* a switch to debootstrap in parallel with Dashjr |
| 228 | +potentially working on getting vmbuilder working again. Long term, |
| 229 | +Fields is working to overhaul the system. |
| 230 | + |
| 231 | +## Comic relief |
| 232 | + |
| 233 | +{% highlight text %} |
| 234 | +<wumpus> #topic dynamic wallet load/unload (promag) |
| 235 | +<instagibbs_> what's the controversy in this topic :) |
| 236 | +<sipa> it should happen, duh |
| 237 | +<sipa> how and when is another :) |
| 238 | +{% endhighlight %} |
| 239 | + |
| 240 | +## Participants |
| 241 | + |
| 242 | + |
| 243 | +| IRC nick | Name/Nym | |
| 244 | +|-----------------|---------------------------| |
| 245 | +| sipa | [Pieter Wuille][] | |
| 246 | +| wumpus | [Wladimir van der Laan][] | |
| 247 | +| jonasschenlli | [Jonas Schnelli][] | |
| 248 | +| luke-jr | [Luke Dashjr][] | |
| 249 | +| BlueMatt | [Matt Corallo][] | |
| 250 | +| promag | [Joao Barbosa][] | |
| 251 | +| jnewbery | [John Newbery][] | |
| 252 | +| jtimon | [Jorge Timón][] | |
| 253 | +| jimpo | [Jim Posen][] | |
| 254 | +| achow101 | [Andrew Chow][] | |
| 255 | +| randolf | [Randolf Richardson][] | |
| 256 | +| instagibbs | [Gregory Sanders][] | |
| 257 | +| cfields | [Cory Fields][] | |
| 258 | +| sdaftuar | [Suhas Daftuar][] | |
| 259 | +| meshcollider | [Samuel Dobson][] | |
| 260 | +| jcorgan | [Johnathan Corgan][] | |
| 261 | +| kanzure | [Bryan Bishop][] | |
| 262 | + |
| 263 | +## Disclaimer |
| 264 | + |
| 265 | +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. |
| 266 | + |
| 267 | +[#11857]: https://github.com/bitcoin/bitcoin/issues/11857 |
| 268 | +[#12560]: https://github.com/bitcoin/bitcoin/issues/12560 |
| 269 | +[#11775]: https://github.com/bitcoin/bitcoin/issues/11775 |
| 270 | +[#10757]: https://github.com/bitcoin/bitcoin/issues/10757 |
| 271 | +[#12874]: https://github.com/bitcoin/bitcoin/issues/12874 |
| 272 | +[#10740]: https://github.com/bitcoin/bitcoin/issues/10740 |
| 273 | +[#11402]: https://github.com/bitcoin/bitcoin/issues/11402 |
| 274 | +[Gitian]: https://github.com/bitcoin-core/docs/blob/master/gitian-building.md |
0 commit comments