fac63eb5ea doc: Remove -whitelistforcerelay from comment (MarcoFalke)
faabd1514f test: Check that peers with forcerelay permission do not get a feefilter message (MarcoFalke)
fad676b8d2 test: Add connect_nodes method (MarcoFalke)
fac6ef4fb2 test: Add test for no net permission (MarcoFalke)
ffff3fe50a test: Replace self.nodes[0].p2p with conn (MarcoFalke)
faccdc8a31 test: remove redundant generate (MarcoFalke)
fab83b934a test: pep-8 p2p_feefilter.py (MarcoFalke)
Pull request description:
ACKs for top commit:
jonatack:
re-ACK fac63eb move-only change of two class member functions in test_framework.py and rebases since my review @ faccf0a per `git range-diff 4b5c919 faccf0a fac63eb`. Verified p2p_feefilter and p2p_permissions functional tests are running 🟢 locally.
Tree-SHA512: 30a1c83baee15a4236d127d199c4f264852045372918d5aa5c09ef3d48041762ce3920ff86ef2466d4b2c792ddf56943d12b16c6dce34c6c5aea2a4af2eb4d49
cc84460c16 test: move sync_blocks and sync_mempool functions to test_framework.py (Roy Shao)
Pull request description:
This PR moves `sync_blocks` and `sync_mempool` out from `test_framework/util.py` to `test_framework/test_framework.py` so they can take contextual information of test framework into account.
* Change all reference callers to call functions from `test_framework.py`
* Remove `**kwargs` which is not used
* Take into account of `timeout_factor` when respecting timeout in function implementations.
* Pass all tests by running `./test/functional/test_runner.py`
fixes#18930
ACKs for top commit:
MarcoFalke:
ACK cc84460c16 , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫
Tree-SHA512: a79b2a3fa842fc26a7aacb834bb2aea88b3049916c0b754e60002a77ce94bb5954e0ea3b436bf268e9295efb62d721dfef263a09339a55c684ac3fda388c275e
931dd47608 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29 [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b733 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd47608
jb55:
ACK 931dd47608
achow101:
ACK 931dd47608
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
22cb303cf0 rpc: add missing space in JSON parsing error message, update test (Jon Atack)
bf53ebef06 test: add multiwallet tests for bitcoin-cli -generate (Jon Atack)
4b859cfff9 cli: add multiwallet capability to GetNewAddress and -generate (Jon Atack)
18f93545a1 test: add tests for bitcoin-cli -generate (Jon Atack)
4818124137 cli: create bitcoin-cli -generate command (Jon Atack)
ff41a36900 cli: extract ParseResult() and ParseError() (Jon Atack)
f4185b26d9 cli: create GenerateToAddressRequestHandler class (Harris)
f7c65a3350 cli: create GetNewAddress() (Jon Atack)
9be7fd35c5 rpc: make generatetoaddress locals const (Jon Atack)
cb00510dba rpc: create rpc/mining.h, hoist default max tries values to constant (Jon Atack)
Pull request description:
This PR continues and completes the work begun in #17700 working on issue #16000 to create a client-side version of RPC `generate`.
Basically, `bitcoin-cli -generate` wraps calling `generatenewaddress` followed by `generatetoaddress [nblocks] [maxtries]` and prints the following:
```
$ bitcoin-cli -generate
{
"address": "bcrt1qn4aszr2y2xvpa70y675a76wsu70wlkwvdyyln6"
"blocks": [
"01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
]
}
$ bitcoin-cli -rpcwallet=wallet-name -generate 3 100
{
"address": "bcrt1q4cunfw0gnsj7g7e6mk0v0uuvvau9mwr09dj45l",
"blocks": [
"7a6650ca5e0c614992ee64fb148a7e5e022af842e4b6003f81abd8baf1e75136",
"01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
"3f8795ec40b1ad812b818c177680841be319a3f6753d4e32dc7dfb5bafe5d00e"
]
}
```
Help doc:
```
$ bitcoin-cli -h | grep -A5 "\-generate"
-generate
Generate blocks immediately, equivalent to RPC generatenewaddress
followed by RPC generatetoaddress. Optional positional arguments
are number of blocks to generate (default: 1) and maximum
iterations to try (default: 1000000), equivalent to RPC
generatetoaddress nblocks and maxtries arguments. Example:
bitcoin-cli -generate 4 1000
```
Quite a bit of test coverage turned out to be needed to cover the change and the different cases (arguments, multiwallet mode) and error-handling.
This PR also improves some things that working on these changes brought to light.
Credit to Harris Brakmić for the initial work in #17700.
ACKs for top commit:
adamjonas:
utACK 22cb303cf0
meshcollider:
utACK 22cb303cf0
Tree-SHA512: 94f67f632fe093d076f614e0ecff09ce7342ac6e424579200d5211a6615260e438d857861767fb788950ec6da0b26ef56dc8268c430012a3b3d4822b24ca6fbf
cc29d1e2c4 [tools] Update clang-format config (John Newbery)
Pull request description:
In some cases, running clang-format has made code _less_ readable by joining declarations and calls for functions with many arguments into very long lines. For example:
```
- size_t getQueueInfo(std::chrono::system_clock::time_point &first,
- std::chrono::system_clock::time_point &last) const;
+ size_t getQueueInfo(std::chrono::system_clock::time_point& first, std::chrono::system_clock::time_point& last) const;
```
(https://github.com/bitcoin/bitcoin/pull/19090#discussion_r431961148)
This change to clang-format would allow arguments/parameters for func declarations/calls to be split over multiple lines, aligned with the opening parens. It does not force args/params to be on new lines (that setting is `BinPackParameters : true`).
ACKs for top commit:
MarcoFalke:
ACK cc29d1e2c4 fine with me
practicalswift:
ACK cc29d1e2c4
Tree-SHA512: a62474925e71aaff41bdce7960fd5ffd64317da810f694d8084080b054708cf71c2ab2ce3111db5a9260d1c1f9e02d59a2ecb5543b1b6172ce085cb42432160a
bc01f7ae05 doc: release note for rpc getaddressinfo removals (Jon Atack)
90e989390e rpc: getaddressinfo RPCResult fixup (Jon Atack)
a8507c99da rpc: remove deprecated getaddressinfo `labels: purpose` (Jon Atack)
645a8653c8 rpc: remove deprecated getaddressinfo `label` field (Jon Atack)
Pull request description:
These were deprecated in #17578 and #17585, with expected 0.21 removal notified in the 0.20 release notes.
```
- The `getaddressinfo` RPC has had its `label` field deprecated
(re-enable for this release using the configuration parameter
`-deprecatedrpc=label`). The `labels` field is altered from returning
JSON objects to returning a JSON array of label names (re-enable
previous behavior for this release using the configuration parameter
`-deprecatedrpc=labelspurpose`). Backwards compatibility using the
deprecated configuration parameters is expected to be dropped in the
0.21 release. (#17585, #17578)
```
ACKs for top commit:
Sjors:
utACK bc01f7a
adamjonas:
utACK bc01f7a
meshcollider:
utACK bc01f7ae05
Tree-SHA512: ae1af381e32c4c3bde8b061a56382838513a9a82c88767843cdeae3a2ab8aa7d8c2e66e106d2b31ea07d74bb80c191a2f842c9aaecc7c5438ad9a9bc66d1b251
e5327f947c [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)
79804fe24b [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost)
Pull request description:
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`.
Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`.
This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.
ACKs for top commit:
achow101:
ACK e5327f947c
meshcollider:
utACK e5327f947c
Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
4d7369125a Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbe Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f91 Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc468123 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce6 Prefer explicit uint160 conversion (Ben Woosley)
Pull request description:
This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.
Inspired by and built on #17924. Commits are small and focused to ease review.
Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".
ACKs for top commit:
achow101:
ACK 4d7369125a
meshcollider:
re-utACK 4d7369125a
Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
d0a3feea73 Change docs for walletcreatefundedpsbt RPC method (Ivan Vershigora)
Pull request description:
`sequence` field in the list of inputs currently marked as "required". Actually it can be omitted and it's value depends on `locktime` and `options.replaceable` fields. Just the same as in `createpsbt` call.
ACKs for top commit:
achow101:
ACK d0a3feea73
Tree-SHA512: 3f429a2c2eea283a47fb5002a99f7e2a5ed6f67df9fd895c1ab938256c48a6497ed6ac2673d8fe8968dfb67b939f4a84570899d9faf52f3abd6ec90c0703d1bd
951bca61d7 tests: feature_backwards_compatibility.py test 0.16 up/downgrade (Andrew Chow)
3a03a11e8c Skip hdKeypath of 'm' (Andrew Chow)
Pull request description:
Previously the seed was stored with keypath 'm' so we need to skip this as well when determining inactive seeds.
Fixes#19051
ACKs for top commit:
Sjors:
ACK 951bca61d7
instagibbs:
re-utACK 951bca61d7
ryanofsky:
Code review ACK 951bca61d7. No significant changes since last review, just updated comment and some test tweaks
Tree-SHA512: 930f77e7097c9cf4f1012e540bd2b1a72fd279262517f10c1531b2ad48c632ef95e0dd4edea81bcc3b3db306479d34e5e79e5d6c4ed31dfa4b77a4231436436e
A templated BaseHash does not allow for automatic conversion, thus
conversions much be explicitly allowed / whitelisted, which will
reduce the risk of unintended conversions.
These types are equivalent, in data etc, so they need only their
data cast across.
Note a function is used rather than a casting
operator as CKeyID is defined at a lower level than script/standard
b83cc0fc94 Fix link error with --enable-debug (Hennadii Stepanov)
Pull request description:
Fixes a link error on master (39bd9ddb87):
```
$ ./configure --enable-debug
$ make
...
bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x0): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet_tool.a(libbitcoin_wallet_tool_a-wallettool.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-salvage.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-walletdb.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): more undefined references to `InitError(bilingual_str const&)' follow
collect2: error: ld returned 1 exit status
```
See:
- https://github.com/bitcoin/bitcoin/pull/19295#issuecomment-645471771
- https://github.com/bitcoin/bitcoin/pull/19295#issuecomment-645487182
ACKs for top commit:
achow101:
Re-ACK b83cc0fc94
Tree-SHA512: f563d978b6725284049449bb0b3a184d356f32e9b63bcadb0ba71352d3d521af3dbb4a7b4fc0a5a620ed99c357e59f62249c10d0defc0cbe7775f2c06791dabe
fa05f44893 ci: Upgrade most ci configs to focal (MarcoFalke)
fad6720891 doc: move doc to ci readme (MarcoFalke)
fa880773b4 ci: Have one config run in xenial to test against python3.5 (MarcoFalke)
fa6ddb2fa1 travis: Always run multiprocess build (MarcoFalke)
Pull request description:
Generally developers compile with recent compilers, so bumping the ci configs to a recent OS should be uncontroversial. Older OSes (especially with compiler sanitizers) need workarounds that can be dropped by running on a more recent OS.
This pull changes the asan sanitizer and the experimental multiprocess build to use focal.
Also, it runs the no_wallet config on xenial to test against python 3.5, according to `doc/dependencies.md`.
Finally, all configs that mimic gitian (win and mac) will stay at bionic.
ACKs for top commit:
Sjors:
ACK fa05f44893, assuming Travis passes
hebasto:
ACK fa05f44893
Tree-SHA512: 55ec56c71ba2280d27c1a8856a1e6c310b1fbf469d5a8a1dde228063e3892e1dd1e51408ecff7a3d77ac2ae018daa9e9bbbb60598cdeaab8c32a146b11b3e7c4
fa2eb3d5d6 ci: Run asan ci config on cirrus (MarcoFalke)
fa93527738 cirrus: Clear dummy task (MarcoFalke)
Pull request description:
Currently it is not possible to use travis in forked repositories due to the 50 minute limit on builds. A fresh build (uncached) of the address sanitizer config takes more than 50 minutes.
One approach to fix this could be to throw away tests until the run time is less than 50 minutes. However, the risk of being blind of failures in the thrown away tests is not worth the gain. Also, to detect them, one has to run the asan configuration nightly and failures could only be detected post-merge.
Another approach would be to ask travis support to raise the limit for a forked repository. This is a tedious and manual one-by-one process, so I'd rather not.
Finally, a different ci provider can be used, since the config files are designed to be platform-agnostic. This is what I picked.
I kept all settings identical to the travis machine for now. Both providers run in the google cloud, so this should be a "move-only".
ACKs for top commit:
hebasto:
ACK fa2eb3d5d6
Tree-SHA512: 159d7dc6f5b24583e941282cdd40465b15db787f0a658a3e81a7b1a22abdb4cb573709b9b5c4465523e0ba0060b17a68fbdbda7a9ecdeb649f31535d377bbe75
3351c91ed4 refactor: Make CScriptVisitor stateless (João Barbosa)
Pull request description:
`CScriptVisitor` was added in 1025440184 (#1357) and the visitor return type was never used. Now `CScriptVisitor` is stateless and `CScript` is the return type.
ACKs for top commit:
MarcoFalke:
ACK 3351c91ed4🏤
sipa:
utACK 3351c91ed4
Tree-SHA512: d158ad2ebe8ea4dc8cc090b943dd66fa5421a84f9443e16ab2d661df38e1a85de16ff13cbaa56924489d8d43cba25fa3cd8b6904bbbcbf356b886ffe8ffba19a
51e9393c1f refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg (Sebastian Falbesoner)
Pull request description:
Follow-up PR for #18533 -- another small step towards getting rid of the confusing "command" terminology. Also see PR #18610 which tackled the functional tests.
ACKs for top commit:
MarcoFalke:
ACK 51e9393c1f
Tree-SHA512: bb6f05a7be6823d5c4eab1d05b31fee944e700946827ad9425d59a3957fd879776c88c606319cbe9832d9451b275baedf913b71429ea3e01e4e82bf2d419e819
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu
item, giving options to sign or broadcast the loaded PSBT as
appropriate, as well as copying the result to the clipboard or saving
it to a file.
fa1904e5f0 net: Remove dead logging code (MarcoFalke)
fac12ebf4f net: Avoid redundant and confusing FAILED log (MarcoFalke)
Pull request description:
Remove a redundant and confusing "FAILED" log message and gets rid of the unused return type in `ProcessMessage`
ACKs for top commit:
jnewbery:
utACK fa1904e5f0
gzhao408:
utACK fa1904e5f0
troygiorshev:
ACK fa1904e5f0
naumenkogs:
utACK fa1904e
Tree-SHA512: bfa553d5efa022727ed17877fb7c08c14849d804fe6d6a7ce172d513857beba35de41ea40b27ff1aedf68b81e2cda7b2a948ac985fcaaf1b6cfb96cce4837c90
f52d403b81 [net] split PushInventory() (John Newbery)
Pull request description:
PushInventory() is currently called with a CInv object, which can be a
MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine
whether to add the hash to setInventoryTxToSend or
vInventoryBlockToSend.
Since the caller always knows what type of inventory they're pushing,
the CInv is wastefully constructed and thrown away, and tx/block relay
is being split out, we split the function into PushTxInventory() and
PushBlockInventory().
ACKs for top commit:
amitiuttarwar:
utACK f52d403b81. nice cleanup, this has bothered me :)
naumenkogs:
utACK f52d403
sipa:
utACK f52d403b81
Tree-SHA512: 331495199a3b1a2620e6a62beb336e494291b725d8fd64bb44726c02e80807f3974ff4f329bb0f059088e65cd7d41eff276c1065806d2dd6e72c5a9f368e82cd
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)
83fd3a6d73 init: use std::thread for ThreadImport() (fanquake)
Pull request description:
[Mentioned](https://github.com/bitcoin/bitcoin/pull/19142#issuecomment-638090759) in #19142, which removed the `boost::interruption_point()`
in `ThreadImport()`.
ACKs for top commit:
hebasto:
ACK 83fd3a6d73, I have reviewed the code and it looks OK, I agree it can be merged.
donaloconnor:
ACK 83fd3a6
laanwj:
Code review ACK 83fd3a6d73
MarcoFalke:
ACK 83fd3a6d73
Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821
da7a83c5ee Remove WalletDatabase::Create, CreateMock, and CreateDummy (Andrew Chow)
d6045d0ac6 scripted-diff: Replace WalletDatabase::Create* with CreateWalletDatabase (Andrew Chow)
45c08f8a7b Add Create*WalletDatabase functions (Andrew Chow)
Pull request description:
Instead of having `Create`, `CreateMock`, and `CreateDummy` being static functions in `BerkeleyDatabase`, move these to standalone functions in `walletdb.cpp`. This prepares us for having different `WalletDatabase` classes.
Part of #18971. This was originally one commit but has been split into 3 to make it (hopefully) easier to review.
ACKs for top commit:
MarcoFalke:
ACK da7a83c5ee🎂
ryanofsky:
Code review ACK da7a83c5ee. Easy review, nice scripted-diff
Tree-SHA512: 1feb7cb3889168c555154bf3701a49095fd6b8cab911d44b7f7efbf6fcee2280ccb3d4afec8a83755b39a592ecd13b90a318faa655c321f87bdabdf1e2312327
PushInventory() is currently called with a CInv object, which can be a
MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine
whether to add the hash to setInventoryTxToSend or
vInventoryBlockToSend.
Since the caller always knows what type of inventory they're pushing,
the CInv is wastefully constructed and thrown away, and tx/block relay
is being split out, we split the function into PushTxInventory() and
PushBlockInventory().
66666d55b1 doc: Mention repo split in the READMEs (MarcoFalke)
faceed753a doc: Add redirect for GUI issues and pull requests (MarcoFalke)
Pull request description:
## 🥅 Goals
Splitting up the GUI (and splitting out modules in general) has been brought up often in recent years. Now that the GUI is primarily connected through (internal) interfaces with the node, it seems an appropriate time to revive this discussion.
Before looking for solutions, we should define a set of goals that we want to achieve. I will start with some ideas to get started and I hope that others will chime in to share and prioritize their goals.
### Separate issue and patch management
It is currently not possible to subscribe to only a subset of modules in Bitcoin Core, or exclude modules from issue and patch notifications. While it is possible to reactively mute conversations in the stream of all ongoing discussions, there is no way to proactively achieve this. Moreover, the list of open issues and pull request will always include GUI related ones by default. Only with [filters](https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+-label%3AGUI) it is possible to hide them.
### More focused review and interests
Long term goals of the GUI are partially unclear #17395 . Bitcoin Core developers are generally fluent on the command line. Thus, they might not be interested or motivated to review improvements to the GUI, which might not affect their workflow on the command line at all. Splitting up the GUI will hopefully attract similar minded people to a project whose primary goal is to build and improve the GUI.
### Maintain high quality assurance
The quality of the GUI (and even more importantly Bitcoin Core in general) must not degrade. This means that code review itself is not negatively affected by splitting the GUI, but also the integration of the GUI into the rest of Bitcoin Core. One issue could arise when arbitrary version-combinations are allowed. We are struggling hard to test against all supported versions of Boost. Making the GUI version another dimension is going to make testing impossible.
### The GUI *is* Bitcoin Core
When a user downloads Bitcoin Core from our website (or another package manager) they expect the GUI to be included. This should not change (at least not as a result of splitting up the GUI into another project).
Similarly, when building Bitcoin Core, the gui should still be built when `--with-gui` is specified.
## 🌳 Proposed solution: Monotree
TLDR. Everything stays the same, the development process for the GUI changes slightly.
Long version:
* An exact mirror of the master development branch is hosted at `bitcoin-core/gui`. The new repository is used to track gui-only issues and pull requests. Global changes that happen to touch gui code still go to the *main* repo.
* All pull requests will be merged into `bitcoin/bitcoin`.
* Decision making process and maintainers will be identical for both repos.
### Disadvantages
* Review activity might decrease?
* It doesn't go far enough. bitcoin/bitcoin#3440 is proposing a modularized Bitcoin Core. The GUI could be an "add-on", connected over RPC or capnproto (bitcoin/bitcoin#10102). Thus, the gui could even be hosted as a subtree or completely separate project.
### Advantages
* Review activity might increase? It is impossible to predict the future, but for example the `libsecp256k1` subtree has a lot of domain specific experts, maintainers and reviewers. I think longer term it makes sense to at least try this route for the gui as well.
* A smaller step is easier to undo when it turns out to come with any unforeseen downsides.
* No substantial changes to the decision making progress.
* Nothing changes in how developers set up their dev environment or how users build from the source. Also, the release binaries and process will stay exactly the same. No version drift. Finally, code sharing between the GUI and Bitcoin Core is not made any harder.
* The organizational side. There are 72 open issues (~14%) and 61 open PRs (~16%) with the GUI label. If moved to its own repo, non-GUI developers wouldn't have to be distracted with GUI-only issues and PRs and GUI enhancements. GUI developers have their own repo to focus on GUI development exclusively.
### Implementation (outstanding TODOs)
* Adjust maintainer merge script https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/57
* Create bitcoin-core/gui repository (empty or with master branch only)
* Assign all existing bitcoin core maintainers to the new repo
* Celebrate? 🥳
* Long-term: Think how long the grace period is for existing GUI related issues and pull requests. Issues can be transferred with a script after a grace period of some months?
ACKs for top commit:
fjahr:
ACK 66666d55b1
Sjors:
ACK 66666d55b1
troygiorshev:
re-ACK 66666d5
practicalswift:
re-ACK 66666d55b1
hebasto:
re-ACK 66666d55b1
Tree-SHA512: 2e1a8de945fa6995583059a2e322621763fccce74a869f9aa750f73546b26350487c4acc4222c03cb3ac1f88e80f0b9d9a3a80a200432fee0d785f52c5cb6174
