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
Previously, we did not include the macOS SDK libc++ headers in our SDK
creation process and instead used whichever libc++ headers shipped with
the clang package we downloaded in depends.
This change adds a script (which works on both GNU/Linux and macOS) to
correctly generate the macOS SDK including the libc++ headers. This can
be thought of as a simplified rewrite of tpoechtrager's script:
d3392f4eae/tools/gen_sdk_package.sh
The location within the SDK where we place the libc++ headers is chosen
such that clang's search path detection logic for sysroots would pick up
the headers properly.
We also document this change.
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
- call disconnect_p2ps() outside of the assert_debug_log scopes
- send messages directly from the p2p conn rather than via nodes[0].p2p
- add an assertion
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
a389ed52e8 walletdb: refactor Read, Write, Erase, and Exists into non-template func (Andrew Chow)
Pull request description:
In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.
The functions `ReadKey`, `WriteKey`, `EraseKey`, and `HasKey` are introduced to handle the actual interaction with the database.
This is mostly a moveonly.
Based on #19290
ACKs for top commit:
ryanofsky:
Code review ACK a389ed52e8. No changes since last review, just non-conflicting rebase
Sjors:
utACK a389ed52e8
MarcoFalke:
ACK a389ed52e8🔳
Tree-SHA512: 73bd2fe9ddc4a132d4db6b97e77f5d5f8aa68b8cb25192384f3bacd826365947763a9eee73672331d34578e3f5ade85ee6aa550ff4d89eb62e482250dd5973e4
