7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
fa1c74fd03 wallet: Remove unused boost::thread_interrupted (MarcoFalke)
fa7b885f51 walletdb: Remove unsed boost/thread (MarcoFalke)
5555d978b0 wallet: Make PeriodicFlush uninterruptible (MarcoFalke)
Pull request description:
The `boost::this_thread::interruption_point()` in the code base currently block the replacement of `boost::thread` with `std::thread`. [1]
Remove them from the wallet because they are either unused or useless.
The feature to interrupt a periodic flush is useless because all wallets have just been flushed 9ccaee1d5e/src/init.cpp (L194) and another flush should be a noop. Also, they will be flushed again shortly after 9ccaee1d5e/src/init.cpp (L285), so even if repeated flushes weren't a noop, doing 3 instead of 2 shouldn't matter too much at this point. Also, the wallet is flushed every two seconds in the worst case, so if this is an expensive operation, that period should be readjusted. (Or bdb should be removed altogether #18916)
[1] Replacement of `boost::thread` with `std::thread` should happen because:
* The boost thread dependency is slow to compile
* Boost thread is less maintained than the standard lib
* Boost thread is mostly redundant to the standard lib
* Global interruption points via exceptions are hard to keep track of during review and easy to get wrong during runtime (e.g. accidental `catch (...)`)
ACKs for top commit:
fanquake:
ACK fa1c74fd03
Tree-SHA512: b166619256de2ef4325480fa1367f68bc9371ad785ec503aed61eab41ba61f1a9807aab25451a24efda3db64855c9ba0025645b98bc58557bc3ec56c5b3297d0
152e8baf08 Use salted hasher instead of nonce in sigcache (Jeremy Rubin)
5495fa5850 Add Hash Padding Microbenchmarks (Jeremy Rubin)
Pull request description:
This PR replaces nonces in two places with pre-salted hashers.
The nonce is chosen to be 64 bytes long so that it forces the SHA256 hasher to process the chunk. This leaves the next 64 (or 56 depending if final chunk) open for data. In the case of the script execution cache, this does not make a big performance improvement because the nonce was already properly padded to fit into one buffer, but does make the code a little simpler. In the case of the sig cache, this should reduce the hashing overhead slightly because we are less likely to need an additional processing step.
I haven't benchmarked this, but back of the envelope it should reduce the hashing by one buffer for all combinations except compressed public keys with compact signatures.
ACKs for top commit:
ryanofsky:
Code review ACK 152e8baf08. No code changes, just rebase since last review and expanded commit message
Tree-SHA512: b133e902fd595cfe3b54ad8814b823f4d132cb2c358c89158842ae27daee56ab5f70cde2585078deb46f77a6e7b35b4cc6bba47b65302b7befc2cff254bad93d
fa7d3a8890 test: Add missing sync_blocks to wallet_hd (MarcoFalke)
eeeed51f58 test: pep-8 wallet_hd (MarcoFalke)
Pull request description:
This fixes the ` test_framework.authproxy.JSONRPCException: non-final (-26)` error when node 1 is one block behind of node 0. (height 122 vs 123)
ACKs for top commit:
promag:
Code review ACK fa7d3a8890.
Tree-SHA512: b549dce2e08c58b949168ed2013bfa176802c963d0d7e890f643c8792da5dade14d91441dfa74372f1f1d34d696e8900a2b60b4861c0ba2dce99f2a633ab27ff
fa9c675591 Limit scope of all global std::once_flag (MarcoFalke)
Pull request description:
`once_flag` is a helper (as the name might suggest) to execute a callable only once. Thus, the scope of the flag does never need to extend beyond where the callable is called. Typically this is function scope.
Move all the flags to function scope to
* simplify code review
* avoid mistakes where similarly named flags are accidentally exchanged
* avoid polluting the global scope
ACKs for top commit:
hebasto:
ACK fa9c675591, tested on Linux Mint 19.3 (x86_64).
promag:
Code review ACK fa9c675591.
Tree-SHA512: 095a0c11d93d0ddcb82b3c71676090ecc7e3de3d5e7a2a63ab2583093be279242acac43523bbae2060b4dcfa8f92b54256a0e91fbbae78fa92d2d49e9db62e57
4f49d5222e gui, refactor: Register Qt meta types in application constructor (João Barbosa)
Pull request description:
Removes a warning when running `QT_QPA_PLATFORM=cocoa src/qt/test/test_bitcoin-qt`.
ACKs for top commit:
jonasschnelli:
Re utACK 4f49d5222e
hebasto:
ACK 4f49d5222e, tested on macOS 10.15.5.
Tree-SHA512: e931a022ba83cb0ef04d82544ebd9b18242f8fc2b41443afce4d5c4222f222e8b3517bdb484a1a4f61377c5dceca067d8ccf250da3a727299448e54bec33ed6e
fac0ed16ec doc: Sync "how to upgrade" with 0.20.0 release notes (MarcoFalke)
fa861794d3 doc: Add release notes for 17219 (MarcoFalke)
Pull request description:
ACKs for top commit:
fanquake:
ACK fac0ed16ec - seems to match the relevant changes in 79606b56b2, 36c1d981d4 and 4c17c85c6c
Tree-SHA512: f04c2b3a1cd094d7f50e3f8db06d726873d2412d651c94bf38d17fd5ee0c47c84480d6b20ec18641888ee88012e7c59e7e794467a0eed1caf44c0569feecc4ca
facef3d413 doc: Explain that anyone can work on good first issues, move text to CONTRIBUTING.md (MarcoFalke)
fae2fb2a19 doc: Expand section on Getting Started (MarcoFalke)
100000d1b2 doc: Add headings to CONTRIBUTING.md (MarcoFalke)
fab893e0ca doc: Fix unrelated typos reported by codespell (MarcoFalke)
Pull request description:
Some random doc changes:
* Add sections to docs, so that they can be linked to
* Explain that anyone (even maintainers) are allowed to work on good first issues
* Expand section on Getting Started slightly
ACKs for top commit:
hebasto:
ACK facef3d413
fanquake:
ACK facef3d413
Tree-SHA512: 8998e273a76dbf4ca77e79374c14efe4dfcc5c6df6b7d801e1e1e436711dbe6f76b436f9cbc6cacb45a56827babdd6396f3bd376a9426ee7be3bb9b8a3b8e383
f898ef65c9 tests: Add fuzzing harness for functions in script/sign.h (practicalswift)
c91d2f0615 tests: Add fuzzing harness for functions in script/sigcache.h (practicalswift)
d3d8adb79f tests: Add fuzzing harness for functions in script/interpreter.h (practicalswift)
fa80117cfd tests: Add fuzzing harness for functions in script/descriptor.h (practicalswift)
43fb8f0ca3 tests: Add fuzzing harness for functions in script/bitcoinconsensus.h (practicalswift)
8de72711c6 tests: Fill fuzzing coverage gaps for functions in script/script.h, script/script_error.h and script/standard.h (practicalswift)
c571ecb071 tests: Add fuzzing helper functions ConsumeDataStream, ConsumeTxDestination and ConsumeUInt160 (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `script/`:
* Add fuzzing helper functions `ConsumeDataStream` and `ConsumeUInt160`
* Fill fuzzing coverage gaps for functions in `script/script.h`, `script/script_error.h` and `script/standard.h`
* Add fuzzing harness for functions in `script/bitcoinconsensus.h`
* Add fuzzing harness for functions in `script/descriptor.h`
* Add fuzzing harness for functions in `script/interpreter.h`
* Add fuzzing harness for functions in `script/sigcache.h`
* Add fuzzing harness for functions in `script/sign.h`
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK f898ef65c9🔉
Tree-SHA512: f6e77b34dc79f23de5fa9e38ac06e6554b5b946ec3e9a67e2bd982e60aca37ce844f785457ef427a5e3b45e31c305456bca8587cc9f4a0b50b3852e39726eb04
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
9a19c9ada5 Always define the raii_event_tests test suite (Craig Andrews)
Pull request description:
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
ACKs for top commit:
MarcoFalke:
ACK 9a19c9ada5🎹
Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176
60ed33904c tests: implement base58_decode (10xcryptodev)
Pull request description:
implements TODO: def base58_decode
ACKs for top commit:
ryanofsky:
Code review ACK 60ed33904c. Just suggested changes since last review. Thank you for taking suggestions!
Tree-SHA512: b3c06b4df041a6d88033cd077a093813a688e42d0b9aa777c715e5fd69cfba7b1bf984428bd98417d3c15232d3d48bc9c163317564f9e1d562db6611c21e2c10
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
fad21a1a7a test: Explain that a bug should be filed when the test fail (MarcoFalke)
Pull request description:
Without a bug report it is harder to fix the issue
ACKs for top commit:
hebasto:
ACK fad21a1a7a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
fanquake:
ACK fad21a1a7a
Tree-SHA512: db194e8f8c0f07b2f4c9ef27e456510959f89da69435cee71605d720e0ad06f18700973f5af25ea31a190b933eb35f2743f014878aa3f8293500e06b4907ebbd
f0d7ed10b4 depends: Propagate only specific CLI variables to sub-makes (Carl Dong)
0a33803f1c depends: boost: Use clang toolset if clang in CXX (Carl Dong)
1ce74bcde3 depends: boost: Split target-os from toolset (Carl Dong)
2d4e480813 depends: boost: Specify toolset to bootstrap.sh (Carl Dong)
3d6603e340 depends: Propagate well-known vars into depends (Carl Dong)
Pull request description:
From: https://github.com/bitcoin/bitcoin/pull/18308#issuecomment-598301117
The following monstrosity is quite useful when invoked inside `depends`, and reviewers can use it to compare the behaviour of this change against master.
```bash
make print-{{,{host,{,{i686,x86_64,riscv64}_}linux}_}{CC,CXX},boost_{cc,cxx}}
```
It would also be helpful to make sure that setting `HOST`, `CC`, and `CXX` does the right thing. The 3 hosts I found offered good coverage were: `{x86_64,i686,riscv64}-linux-gnu`. As we special-case the `x86_64` and `i686` hosts in `depends/hosts/linux.mk`, and `riscv64` is a sanity check for a non-special-cased host.
ACKs for top commit:
hebasto:
ACK f0d7ed10b4, tested on Linux Mint 19.3 (x86_64):
practicalswift:
ACK f0d7ed10b4 -- patch looks correct
laanwj:
Code review and concept ACK f0d7ed10b4
ryanofsky:
Code review ACK f0d7ed10b4. Changes since last review: adding comment explaining check for predefined make variables, dropping freetype commit, adding commit whitelisting overrides for recursive makes
Tree-SHA512: b6b8e76f713c26a0add6cd685824e2f5639109236ee9f89338f7c79cb1b1f2c3897bfb62b80b023d6d1943b5a6eb282a2f827f1f499c5e556eca015d6635fa65
f871f15c9d scripted-diff: replace gArgs with argsman (glowang)
357f02bf29 Create a local class inherited from BasicTestingSetup with a localized args manager and put it into the getarg_tests namespace (glowang)
Pull request description:
Replaced the global argsManager gArgs with a locally defined one in getarg_tests. This is to avoid confusion in arg settings between the test's ArgsManager and the #18804
ACKs for top commit:
MarcoFalke:
ACK f871f15c9d
ryanofsky:
Code review ACK f871f15c9d. Changes look good and thanks for updating. In future would recommend using clang-format-diff and following [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) notes, because it's atypical to indent namespace content, or indent protected keywords or put spaces around ::. Also it's fragile to define test setup class in a namespace, but test setup methods outside of the namespace and inside the test fixture instead. Would be simpler to just define the testing setup completely before using it without a namespace like: 8ad5f1c376/src/test/rpc_tests.cpp (L23) and it would have been a slightly smaller change too.
Tree-SHA512: 016594639396d60667fadec8ea80ef7af634fbb2014c704f02406fe3251c5362757c21f1763d8bdb94ca4a3026ab9dc786a92a9a934efc8cd807655d9deee779
189ae0c38b util: dedup code in callers of serviceFlagToStr() (Vasil Dimov)
fbacad1880 util: simplify the interface of serviceFlagToStr() (Vasil Dimov)
Pull request description:
Don't take two redundant arguments in `serviceFlagToStr()`.
Introduce `serviceFlagsToStr()` which takes a mask (with more than one
bit set) and returns a vector of strings.
As a side effect this fixes an issue introduced in
https://github.com/bitcoin/bitcoin/pull/18165 due to which the GUI could
print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]`
instead of `NETWORK & WITNESS`.
ACKs for top commit:
MarcoFalke:
ACK 189ae0c38b
jonasschnelli:
Tested ACK 189ae0c38b
Tree-SHA512: 000c490f16ebbba04458c62ca4ce743abffd344d375d95f5bbd5008742012032787655db2874b168df0270743266261dccf1693761906567502dcbac902bda50
Don't take two redundant arguments in `serviceFlagToStr()`.
As a side effect this fixes an issue introduced in
https://github.com/bitcoin/bitcoin/pull/18165 due to which the GUI could
print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]`
instead of `NETWORK & WITNESS`.
da73f1513a qt: Fix shutdown when waitfor* cmds are called from RPC console (Hennadii Stepanov)
Pull request description:
On master (7eed413e72), if the GUI has been started with`-server=1`, `bitcoin-qt` hangs on shutdown during calling any of the `waitfor*` commands in the GUI RPC console.
This PR suggests minimal changes to fix this bug.
Fix#17495
ACKs for top commit:
jonasschnelli:
utACK da73f1513a
Tree-SHA512: 469f5332945a5f2c57d19336cda5df79b123ccc494aea6d58a85eb1293be52708b2b9c5bb6bc2c402a90b7b4e9e8d7ab8fe84cf201cf7ce612c9290c57e43681
9760293ce6 wallet: Fix for exported confirmation field in payment to self transactions (Ben Carman)
Pull request description:
Closes#3455
ACKs for top commit:
jonasschnelli:
Tested ACK 9760293ce6
Tree-SHA512: 8207768771ad787f716b966c4aa7aeef2da8a602e32e3510e41c7b49ec5ec679a3835d248be5016d4b37764f9914846f7c41c11cf48cddb617cb7ef831318fd7
96954d1794 DNS seeds: don't query DNS while network is inactive (Anthony Towns)
fa5894f7f5 DNS seeds: wait for 5m instead of 11s if 1000+ peers are known (Anthony Towns)
Pull request description:
Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active.
Fixes#15434
ACKs for top commit:
fanquake:
ACK 96954d1794 - Ran some tests of different scenarios. More documentation is being added in #19084.
ariard:
Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work.
Sjors:
tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with `-debug=addrman` and change `DNSSEEDS_DELAY_MANY_PEERS` to something lower to test the behaviour, as well as renaming `peers.dat` to test the peer threshold.
naumenkogs:
utACK 96954d1794
Tree-SHA512: 73693db3da73bf8e76c3df9e9c82f0a7fb08049187356eac2575c4ffa455f76548dd1c86a11fc6beea8a3baf0ba020e047bebe927883c731383ec72442356005
8e08d00598 qt: Use parent-child relation to manage lifetime of OptionsModel object (Hennadii Stepanov)
Pull request description:
Both `BitcoinApplication` and `OptionsModel` classes are derived from the `QObject` class, therefore a parent-child relation could be established to manage the lifetime of an `OptionsModel` object:
5236b2e267/src/qt/optionsmodel.cpp (L29-L30)
This PR does not change behavior.
ACKs for top commit:
jonasschnelli:
utACK 8e08d00598
promag:
ACK 8e08d00598.
Tree-SHA512: 0223dddf5ba28b0bfaefeda1b03b4ff95bf7e7d0c1e7b32368171e561813e22129f2a664f09279fa3b4fa63259b7680d55aa3fe66db9c7ae0039b7f529777ec3
c31bc5bcfd Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function (Luke Dashjr)
cea91a1e40 Bugfix: GUI: Use unsigned long long type to avoid implicit conversion of MSB check (Luke Dashjr)
Pull request description:
Side effect: this results in the RPC showing unknown service bits as "UNKNOWN[n]" like the GUI.
Note that there is no common mask-to-`vector<string>` function because both GUI and RPC would need to iterate through it to convert to their desired target formats.
ACKs for top commit:
jonasschnelli:
utACK ~~cea91a1e40e12029140ebfba969ce3ef2965029c~~ c31bc5bcfd
Tree-SHA512: 32c7ba8ac7ef2d4087f4f317447ae93a328ec9fb9ad81301df2fbaeeb21a3db7a503187a369552b05a9414251b7cf8e15bcde74c1ea2ef36591ea7ffb6721f60
a06e845e82 BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. (furszy)
2f867203b0 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (furszy)
Pull request description:
Rationale:
The height based polling in the GUI is an issue on chain reorgs. Any new tip signal with the same height as the one that it's cached in the model was not triggering the GUI update (interpreting it as the same same block when it could receive a different one). Ending up with bad information presented in the GUI.
This PR essentially changes the last cached height to be a last cached block hash.
---
Old historical information of this PR.
As the tip height is cached and updated via signaling in clientModel, there is no need to continue locking `cs_main` on every balance poll (`m_node.getNumBlocks()` method call).
Extra topic:
Would suggest to change the `cachedNumBlocks` field inside `walletModel` to a more understandable name, maybe `nLastBalanceUpdateHeight`.
And finally, this will have the equal height reorg issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304), whatever is presented to fix it, this should use the same flow too.
**[Edit - 24/01/2020]**
Have added #[17905](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304) comment fix here too.
ACKs for top commit:
jonasschnelli:
utACK a06e845e82 - it would be great to have QT unit tests (in this case for a reorg) that either automatically inspect the window content based on accessibility and tests for expected values or at least allow for quick manual re-testing (screenshots, automatically create UI situations).
hebasto:
re-ACK a06e845e82, suggested style changes implemented since the [previous](https://github.com/bitcoin/bitcoin/pull/17993#pullrequestreview-417249705) review.
ryanofsky:
Code review ACK a06e845e82. A lot of changes since the last review: rebase after sync_state introduction #18152 and tryGetBalances revert #18587, reverting getLastBlockTime change, fixing spacing and initializations and renaming some variables
Tree-SHA512: 835e587a8296df9899cccd7b3e598a5970942b640e432e6a32de0b4eaea5b40f9271258f089ec033595311707b74a0f7187ecf8ed397c713e1153e2714072975
c4ea501e96 qt: Hide non PKHash-Addresses in signing address book (Emil Engler)
Pull request description:
[Video Demo](https://www.youtube.com/watch?v=T-Rp2pFRmzY)
This PR hides all non PKHash addresses in the signing GUI in the Address Book when it is opened through the signing dialog, as non PKHash addresses are useless there.
ACKs for top commit:
jonasschnelli:
Code Review ACK c4ea501e96
Tree-SHA512: e321d45e15534b2d68da5a1297b1c7551cdd784f03203f54c9385c2ce0bb2b7316c09f9e8c3eb41bfa1e7207ecc94c8ed08f012e2d6c117b803996ade26feb2f
