9952242c03fe587b5dff46a9f770e319146103bf build: improve builtin_clz* detection (fanquake)
Pull request description:
Fixes#19402.
The way we currently test for `__builtin_clz*` support with `AC_CHECK_DECLS` does not work with Clang:
```bash
configure:21492: clang++-10 -std=c++11 -c -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
conftest.cpp💯10: error: builtin functions must be directly called
(void) __builtin_clz;
^
1 error generated.
```
This also removes the `__builtin_clz()` check, as we don't actually use it anywhere, and it's trvial to re-add detection if we do start using it at some point. If this is controversial then I'll add a test for it as well.
ACKs for top commit:
sipa:
ACK 9952242c03fe587b5dff46a9f770e319146103bf
laanwj:
ACK 9952242c03fe587b5dff46a9f770e319146103bf
Tree-SHA512: 695abb1a694a01a25aaa483b4fffa7d598842f2ba4fe8630fbed9ce5450b915c33bf34bb16ad16a16b702dd7c91ebf49fe509a2498b9e28254fe0ec5177bbac0
0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 qt: Do not truncate node flag strings in debugwindow.ui peers details tab. (saibato)
Pull request description:
Fix: When fiddling around with new node flags other than the usual.
I saw that not all possible node flag strings i.e. the UNKNOWN[..] where
visible in peers details tab.
Since v18.2 fixed size was set to 300 and sliding is thereby limited.
A fix on my old linux cruft and small screen was to set minimumSize width to -1 or 0.
Qt will then autosize the slider to the max string length.
Thereby i had full display of all flags inclusive sliding without to fullscreen the window.
Not sure if this is even an issue for those who can afford big screens or high res macs?
Feedback welcome.
BTW: nice side effect now again easy to scroll trough long version names of the node.
can't wait to see strings like /Satoshi:0.23.99/NOX2NOX4NOX32 or what ever fits in the version string.
ACKs for top commit:
hebasto:
ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
promag:
Tested ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 on macos.
Tree-SHA512: a1601b5e35f10b1fd9407b28142ca00c1b985a822be5d23be4d7d3376211450f06e17f962c44b8b40977f8f8bbbb701cac1c5abb4afb3618da76385dfac848a3
4d7369125a82214ea42b808a32b71b315a5c3c72 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbed32438bdb32623af6e06f13ecd35e4 Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859db37b1775e2180e5be032fc4fdf483 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd6c174ff3f6406a619550d2f6c19360 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f913967703b404747133be67cf2f4feac Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144480d7b170e49087df162989d31cd20 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc46812334074d2c77a6233e8a961cd0785872 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce605984094c5552877cb99bc81654f2c 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 4d7369125a82214ea42b808a32b71b315a5c3c72
meshcollider:
re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72
Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
25f3554351a99a0a695fbd2a6a0f293b3adc3d98 scripted-diff: Make SeparatorStyle a scoped enum (Hennadii Stepanov)
Pull request description:
This PR is [split](https://github.com/bitcoin/bitcoin/pull/17877#issuecomment-644751515) from https://github.com/bitcoin/bitcoin/pull/17877 and makes `BitcoinUnits::SeparatorStyle` a scoped enum.
ACKs for top commit:
MarcoFalke:
review ACK 25f3554351a99a0a695fbd2a6a0f293b3adc3d98 🚐
Tree-SHA512: 578f1340a476cf79faa109a83815d3c75e26d9c18873e653d7624b52428ccb2677293116db0a60ae14c949d63b64988fc5a39c7184c2352b87b00e8ddaaaf474
0f8f51544558d204a4e9d0607b0f375150529637 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae
d49612f98add29066817b7c808b76c2d728948e5 Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0355662481a7d181a0a458284936d53eb1 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa481cefbe12c50f344bae190c0d95fb489 refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov)
Pull request description:
This is one more step for consistent usage of `bilingual_str`.
No new translation messages are defined.
ACKs for top commit:
laanwj:
Code review ACK d49612f98add29066817b7c808b76c2d728948e5
MarcoFalke:
ACK d49612f98add29066817b7c808b76c2d728948e5 🌂
Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
e2bab2aa162ae38b2bf8195b577c982402fbee9d multiprocess: add multiprocess travis configuration (Russell Yanofsky)
603fd6a2e708c04ef6c9880f89d0a4cbaa6fc7c5 depends: add MULTIPROCESS depends option (Russell Yanofsky)
5d1377b52bfcd4edf8553aaf332bfeb92fc554cc build: multiprocess autotools changes (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR consists of build changes only. It adds an `--enable-multiprocess` autoconf option (off by default and marked experimental), that builds new `bitcoin-node` and `bitcoin-gui` binaries. These currently function the same as existing `bitcoind` and `bitcoin-qt` binaries, but are extended in #10102 with IPC features to execute node, wallet, and gui functions in separate processes.
In addition to adding the `--enable-multiprocess` config flag, it also adds a depends package and autoconf rules to build with the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library, and it adds new travis configuration to exercise the build code and run functional tests with the new binaries.
The changes in this PR were originally part of #10102 but were moved into #16367 to be able to develop and review the multiprocess build changes independently of the code changes. #16367 was briefly merged and then reverted in #18588. Only change since #16367 has been dropping the `native_boost.mk` depends package which was pointed out to be no longer necessary in https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-596484337 and https://github.com/bitcoin/bitcoin/pull/18588#pullrequestreview-391765649
ACKs for top commit:
practicalswift:
ACK e2bab2aa162ae38b2bf8195b577c982402fbee9d
Sjors:
tACK e2bab2aa162ae38b2bf8195b577c982402fbee9d on macOS 10.15.4
hebasto:
ACK e2bab2aa162ae38b2bf8195b577c982402fbee9d, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: b5a76eab5abf63d9d8b6d628cbdff4cc1888eef15cafa0a5d56369e2f9d02595fed623f4b74b2cf2830c42c05a774f0943e700f9c768a82d9d348cad199e135c
3913d1e8c1f604bdd622d5e81e5077ef52b30466 qt: Drop buggy TableViewLastColumnResizingFixer class (Hennadii Stepanov)
Pull request description:
In Qt 5 the last column resizing with dragging its left edge works out-of-the-box.
The current `TableViewLastColumnResizingFixer` implementation could put the last column content out of the view port and confuse a user:
Historical context:
- https://github.com/bitcoin/bitcoin/pull/2862
- https://github.com/bitcoin/bitcoin/pull/3626
- https://github.com/bitcoin/bitcoin/pull/3738
- https://github.com/bitcoin/bitcoin/pull/3920#205 is a nice addition.
ACKs for top commit:
jarolrod:
ACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466, tested on macOS 11.1 Qt 5.15.2
Talkless:
tACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466, tested on Debian Sid. Can confirm that behavior in previous commit does not produce scroll bar, last column gets "hidden". This PR makes clear that there's more to see in the view.
promag:
Tested ACK 3913d1e8c1f604bdd622d5e81e5077ef52b30466 on macos.
Tree-SHA512: 12582dfce54bb1db3d9934ae092e305d32e9760cc99b0265322e161fa7f54b7d6fb6cefedf700783f767d5c3a56a8545c8d2f5ade66596c4e67b8a5287063e8a
9266f7497f256d780178829e0f3a29ddaeb794ba util: Use std::chrono for time getters (MarcoFalke)
3c2e16be22ae04bf56663ee5ec1554d0d569741b time: add runtime sanity check (Cory Fields)
Pull request description:
I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.
ACKs for top commit:
practicalswift:
Tested ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
laanwj:
Code review ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
Tree-SHA512: 5471a60e65e9fa8ef48320743ef637f1d162724e717e0f5509118e1e5732fc0844656a9c09d3d1300eb657dcc7a1e1e67305d8c9ef959c63be67393607dd4ceb
fa650ca7f19307a9237e64ac311488c8947fc12a Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd2aed8380cc9b9e52791f662aecd6a6 test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f19307a9237e64ac311488c8947fc12a: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f19307a9237e64ac311488c8947fc12a, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
fa362064e383163a2585ffbc71ac1ea3bcc92663 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
glozow:
ACK fa362064e3🧸
jnewbery:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
74d23bf7fbb6169ec658c36af57cd1f937823d9c rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)
Pull request description:
It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.
Closes#5638
ACKs for top commit:
jonatack:
ACK 74d23bf7fbb6169ec658c36af57cd1f937823d9c
Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
8775691383ff394b998232ac8e63fac3a214d18b Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" (Luke Dashjr)
Pull request description:
The SendConfirmationDialog is used for bumping the fee, where "Send" doesn't really make sense
Originally https://github.com/bitcoin/bitcoin/pull/17463, but rewritten here much simpler based on other merged changes.
ACKs for top commit:
hebasto:
ACK 8775691383ff394b998232ac8e63fac3a214d18b, tested on Linux Mint 20.1 (x86_64, Qt 5.12.8):
Tree-SHA512: 3953cc9c09613c9a629def8b4dc061b537f148ddcb378430645602e0be0f3a9f1cff083aa685b94b2e9372300d02ec97e0d9ea89db6e3c6feec86795090f0f77
3eb94ec81b72b14f72a1f6ce5c9aa24476df755a sync: Use decltype(auto) return type for WITH_LOCK (Carl Dong)
Pull request description:
> Now that we're using C++17, we can use the decltype(auto) return type
> for functions and lambda expressions.
>
> As demonstrated in this commit, this can simplify cases where previously
> the compiler failed to deduce the correct return type.
>
> Just for reference, for the "assign to ref" cases fixed here, there are
> 3 possible solutions:
>
> - Return a pointer and immediately deref as used before this commit
> - Make sure the function/lambda returns declspec(auto) as used after
> this commit
> - Class& i = WITH_LOCK(..., return std::ref(...));
>
> -----
>
> References:
> 1. https://en.cppreference.com/w/cpp/language/function#Return_type_deduction
> 2. https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts
> 3. https://en.cppreference.com/w/cpp/language/auto
> 4. https://en.cppreference.com/w/cpp/language/decltype
>
> Explanations:
> 1. https://stackoverflow.com/a/21369192
> 2. https://stackoverflow.com/a/21369170
Thanks to sipa and ryanofsky for helping me understand this
ACKs for top commit:
jnewbery:
utACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a
hebasto:
ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings:
ryanofsky:
Code review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a
Tree-SHA512: 5f55c7722aeca8ea70e5c1a8db93e93ba0e356e8967e7f607ada38003df4b153d73c29bd2cea8d7ec1344720d37d857ea7dbfd2a88da1d92e0e9cbb9abd287df
aaaa9878405f3f38f4f61c00feca110d7f9ca481 refactor: Use C++17 std::array deduction for ALL_FEE_ESTIMATE_HORIZONS (MarcoFalke)
fa39cdd072c91eac70cda04b8b26681611f94cb7 refactor: Use C++17 std::array deduction for OUTPUT_TYPES (MarcoFalke)
Pull request description:
With the new C++17 array deduction rules, an array encompassing all values in an enum can be specified in the same header file that specifies the enum. This is useful to avoid having to repeatedly enumerate all enum values in the code. E.g. the RPC code, but also the fuzz code.
ACKs for top commit:
theStack:
cr ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481 ⚙️
fanquake:
ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481
Tree-SHA512: b71bd98f3ca07ddfec385735538ce89a4952e418b52dc990fb160187ccef1fc7ebc139d42988b6f7b48df24823af61f803b83d47fb7a3b82475f0c0b109bffb7
8008ef770f3d0b14d03e22371314500373732143 qt: unlock wallet "OK" button bugfix (Michael Dietz)
Pull request description:
When trying to send a transaction from an encrypted wallet, the ask
passphrase dialog would not allow the user to click the "OK" button
and proceed. Therefore it was impossible to send a transaction
through the gui. It was not enabling the "OK" button after the
passphrase was entered by the user, because it was using the same
form validation logic as the "Change passphrase" flow.
I reported this in a comment in https://github.com/bitcoin-core/gui/issues/136. But then I realized this seems to be a flat out bug.
ACKs for top commit:
MarcoFalke:
review ACK 8008ef770f3d0b14d03e22371314500373732143
hebasto:
ACK 8008ef770f3d0b14d03e22371314500373732143, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: cc09b34c7f3aea09729e1c7ccccff05dc11fec56fee2ad369f2d862979572b1edd8b7e738ffe6e91d35d071b819b0c3e0f5d48bf5e27427a80af4a28893f8aaf
11e79084845a78e2421ea3abafe0de5a54ca2bde prevector: only allow trivially copyable types (Martin Leitner-Ankerl)
Pull request description:
prevector uses `memmove` to move around data, that means it can only be used with types that are trivially copyable. That implies that the types are trivially destructible, thus the checks for `is_trivially_destructible` are not needed.
ACKs for top commit:
w0xlt:
ACK 11e7908484
MarcoFalke:
review ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde 🏯
ajtowns:
ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde -- code review only
Tree-SHA512: cbb4d8bfa095100677874b552d92c324c7d6354fcf7adab2ed52f57bd1793762871798b5288064ed1af2d2903a0ec9dbfec48d99955fc428f18cc28d6840dccc
bdc6881e2f796f4a9a5873826219e24f17a96a7c wallet: Change log interval to use `steady_clock` (w0xlt)
Pull request description:
This refactors the log interval variables to use `steady_clock` as it is best suitable for measuring intervals.
ACKs for top commit:
laanwj:
This makes sense. Code review ACK bdc6881e2f796f4a9a5873826219e24f17a96a7c
dunxen:
Code review ACK bdc6881
Tree-SHA512: 738b4aa45cef01df77102320f83096a0a7d0c63d7fcf098a8c0ab16b29453a87dc789c110105590e1e215d03499db1d889a94f336dcb385b6883c8364c9d39b7
fa4652ce5995ace831b6a4d3125bfcac9563ff6f Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake)
Pull request description:
Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference.
All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines.
ACKs for top commit:
pk-b2:
ACK fa4652ce59
jonatack:
Code review ACK fa4652ce5995ace831b6a4d3125bfcac9563ff6f rebased to master, debug build, unit tests
vincenzopalazzo:
ACK fa4652ce59
Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
e5485e8e4be7f2ee0671f58c3dcce35c68ba0ee0 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee509025d92db5311c3f5df3b61c09cad24f validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
fad6d4f952373690ef16ce27b0926c0ab762066a Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb2287b2a1a4eb7224b424f12fe59302e9 Remove duplicate static_asserts (MarcoFalke)
Pull request description:
No need to go from `arith_uint256`->`uint256` when a `uint256` can be constructed right away.
ACKs for top commit:
laanwj:
Code review ACK fad6d4f952373690ef16ce27b0926c0ab762066a
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
3ec6504a2e5b4afb7a2719a82191e0b96fe23214 qt: Do not use `QKeyEvent` copy constructor (Hennadii Stepanov)
Pull request description:
This PR is preparation for [Qt 6](https://github.com/bitcoin/bitcoin/pull/24798), and it fixes an experimental build with Qt 6.2.4 as copying of `QEvent` has been [disabled](19f9b0d5f5) in Qt 6.0.0.
ACKs for top commit:
w0xlt:
tACK 3ec6504a2e on Ubuntu 21.10, Qt 5.15.2
shaavan:
reACK 3ec6504a2e5b4afb7a2719a82191e0b96fe23214
Tree-SHA512: 583a9dad0c621d9f02f77ccaa9f55ee79e12e3c47f418911ef2dfe0de357d772d1928ae3ec19b6f0c0674da858bab9d4542a26cc14b06ed921370dfeabd1c194
99a6b699cd650f13d7200d344bf5e2d4b45b20ac Fix race condition for SetBannedSetDirty() calls (Hennadii Stepanov)
83c76467157bbca023bffda0f0bc2f01eb76a040 Avoid calling BanMan::SweepBanned() twice in a row (Hennadii Stepanov)
33bda6ab87cc1b569e96da337296eb3e9ce6db1a Fix data race condition in BanMan::DumpBanlist() (Hennadii Stepanov)
5e20e9ec3859205c220867ca49efb752b8edaacc Prevent possible concurrent CBanDB::Write() calls (Hennadii Stepanov)
Pull request description:
This PR split from bitcoin/bitcoin#24097 with some additions. This makes the following switch from `RecursiveMutex` to `Mutex` a pure refactoring.
See details in commit messages.
ACKs for top commit:
w0xlt:
reACK 99a6b69
shaavan:
ACK 99a6b699cd650f13d7200d344bf5e2d4b45b20ac
Tree-SHA512: da4e7268c7bd3424491f446145f18af4ccfc804023d0a7fe70e1462baab550a5e44f9159f8b9f9c7820d2c6cb6447b63883616199e4d9d439ab9ab1b67c7201b
c62d763fc313585d79ad833c9d729f6acf2652aa Necessary improvements to make configure work without libevent installed (Perlover)
091ccc38c2e589b649648cbcc99aca4802f98775 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover)
Pull request description:
The second parameter of evhttp_connection_get_peer in libevent already has type as `const char **`
The compilation of bitcoind with the fresh libevent occurs errors
Details: https://github.com/bitcoin/bitcoin/issues/23606
ACKs for top commit:
laanwj:
Code review ACK c62d763fc313585d79ad833c9d729f6acf2652aa
luke-jr:
tACK c62d763fc313585d79ad833c9d729f6acf2652aa
Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83
64e1ddd255771e57a88a20f07dbde04a83bf0c75 log: call LogPrint only once with time data samples (Martin Zumsande)
Pull request description:
When timedata samples are logged, `LogPrint()` is currently invoked multiple times on the same log entry.
This can lead to chaos in the log when other threads log concurrently, as in this example which motivated this PR:
```
2021-09-20T00:28:57Z -48 -26 -11 -8 -6 Addrman checks started: new 37053, tried 83, total 37136
2021-09-20T00:28:57Z -3 -1 -1 -1 -1 +0 | nTimeOffset = -3 (+0 minutes)
```
Fix this by building the log message in a string and logging it one `LogPrint()` call. I also changed the wording slightly so that it becomes understandable what is being logged, example:
```
2021-09-21T21:03:24Z time data samples: -43 -18 -12 -4 -1 -1 +0 +0 +268 | median offset = -1 (+0 minutes)
```
ACKs for top commit:
jnewbery:
Tested ACK 64e1ddd255
laanwj:
Tested ACK 64e1ddd255771e57a88a20f07dbde04a83bf0c75, new message lgtm
Tree-SHA512: ffb7a93166cc8fd6a39200b9e03a9d1e8e975b7ded822ccddd015f553258b991162a5cb867501f426d3ebcfef4f32f0e06e17b18e6b01486b967595d102f8379
cc3044ccdbefa9fae58d1762477e377883b39c5e fix misleading comment about call to non-existing function (pox)
Pull request description:
The comment seems to be describing the subsequent call to `SyncTransaction` but refers to it as `SyncNotifications`, which is not any function currently in the codebase.
It's best to just remove the "what" aspect of the comment and focus on the "why", which also reduces the risk of similar documentation errors in the future, in case the function ever gets renamed, for example.
ACKs for top commit:
laanwj:
ACK cc3044ccdbefa9fae58d1762477e377883b39c5e
Tree-SHA512: 882ff17836ef585a603dc504f3dd21f56f682e49b28a0998f23fd16025826fbb083b7978db3ee70d0e0ff2c86fd6c3fd99a2361e5d45c765fdc5822c5f14c0a7
198fff88f385e090b57a0ee902719bcc22a6b86b GUI: Define MAX_DIGITS_BTC for magic number in BitcoinUnits::format (Luke Dashjr)
Pull request description:
A magic number snuck in with https://github.com/bitcoin/bitcoin/pull/16432
ACKs for top commit:
hebasto:
ACK 198fff88f385e090b57a0ee902719bcc22a6b86b, I have reviewed the code and it looks OK, I agree it can be merged.
kristapsk:
utACK 198fff88f385e090b57a0ee902719bcc22a6b86b
Tree-SHA512: 78dc23c2ae61bac41e5e34eebf57274599cb2ebb0b18d46e8a3228d42b256a1bc9bb17091c748f0f692ef1c4c241cfbd3e30a12bcd12222a234c1a9547ebe786
2ea62cae483b764e30f61c06d8ac65755bbd864c Improve docs about feeler connections (Gleb Naumenko)
Pull request description:
"feeler" and "test-before-evict" are two different strategies suggest in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://www.usenix.org/system/files/conference/usenixsecurity15/sec15-paper-heilman.pdf). In our codebase, we use `ConnType::FEELER` to implement both.
It is confusing, up to the point that our documentation was just incorrect.
This PR:
- ~clarifies this aspect by renaming "ConnType::FEELER" to "ConnType::PROBE", meaning that this connections only probes that the node is operational, and then disconnects.~
- fixes the documentation
ACKs for top commit:
amitiuttarwar:
ACK 2ea62cae48. thank you!
practicalswift:
ACK 2ea62cae483b764e30f61c06d8ac65755bbd864c
Tree-SHA512: c9c03c09eefeacec28ea199cc3f697b0a98723f2f849f7a8115edc43791f8165e296e0e25a82f0b5a4a781a7de38c8954b48bf74c714eba02cdc21f7460673e5
## Issue being fixed or feature implemented
Client version string is inconsistent. Building `v20.0.0-beta.8` tag
locally produces binaries that report `v20.0.0-beta.8` version but
binaries built in guix would report
`v20.0.0rc1-g3e732a952226a20505f907e4fd9b3fdbb14ea5ee` instead. Building
any commit after `v20.0.0-beta.8` locally would result in versions like
`v20.0.0rc1-8c94153d2497` which is close but it's still yet another
format. And both versions with `rc1` in their names are confusing cause
you'd expect them to mention `beta.8` instead maybe (or is it just me?
:D ).
## What was done?
Change it so that the version string would look like this:
on tag: ~`v20.0.0-beta.8-dev` or `v20.0.0-beta.8-gitarc`~
`v20.0.0-beta.8`
post-tag: ~`v20.0.0-beta.8-1-gb837e08164-gitarc`~
`v20.0.0-beta.8-1-gb837e08164`
post-tag format is
`recent tag`-`commits since that tag`-`g+12 chars of commit hash`-`dirty
(optional)` ~-`dev or gitarc`~
~`dev`/`gitarc` suffixes should help avoiding confusion with the release
versions and they also indicate the way non-release binaries were
built.~
Note that release binaries do not use any of this, they still use
`PACKAGE_VERSION` from `configure` like before.
Also, `CLIENT_VERSION_RC` is no longer used in this setup so it was
removed.
Few things aren't clear to me yet:
1. Version bump in `configure.ac` no longer affects the reported version
(unless it's an actual release). Are there any downsides I might be
missing?
2. Which tag should we use on `develop` once we bump version in
configure? `v21.0.0-init`? `v21.0.0-alpha1`?
3. How is it going to behave once `merge master back into develop` kind
of PR is merged? E.g. say `develop` branch is on `v21.0.0-alpha1` tag
and we merge v20.1.0 from `master` back into it. Will this bring
`v20.1.0` release tag into `develop`? Will it become the one that will
be used from that moment? If so we will probably need another tag on
`develop` every time such PR is merged e.g. `v21.0.0-alpha2` (or
whatever the next number is).
Don't think these are blockers but would like to hear thoughts from
others.
## How Has This Been Tested?
Built binaries locally, built them using guix at a specific tag and at
some commit on top of it.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
We should avoid return by reference; especially return by reference with
a bool flag indicating validity.
## What was done?
Instead we use a std::optional
## How Has This Been Tested?
Unit tests pass
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
9b4fa0af40cd88ed25dd77962235fbf268bdcaa7 net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server) (practicalswift)
Pull request description:
Exit with error message if `-proxy` is specified without arguments (instead of continuing without proxy server).
Continuing without a proxy server when the end-user has specified `-proxy` may result in accidental loss of privacy. (The end-user might think he/she is using a proxy when he/she is not.)
Before this patch:
```
$ src/bitcoind -proxy
…
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -listen=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -upnp=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
…
2020-09-23T00:24:33Z init message: Starting network threads...
```
`bitcoind` is now running *without* a proxy server (`GetProxy(…, …) == false`, `HaveNameProxy() == false`, etc.).
Note that the "-proxy set" log messages above which the end-user might interpret as "good, my traffic is now routed via the proxy".
After this patch:
```
$ src/bitcoind -proxy
Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.
$ echo $?
1
```
ACKs for top commit:
laanwj:
re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7
kristapsk:
ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7, I have tested the code.
hebasto:
re-ACK 9b4fa0af40cd88ed25dd77962235fbf268bdcaa7
Tree-SHA512: 4ba7a011991699a54b5bb87ec68367c681231bf5dcd36f8c89ff9ddc2e8d29df453817b7e362597e652ad6b341a22b7274be0fd78d435e5f0fd8058e5221c4ce
916d3596c493fec44da86aeb92b61eafeea0b596 help: Generate checkpoint height from chainparams (Luke Dashjr)
Pull request description:
Not sure if this is worth putting in Core, but might as well until checkpoints are removed entirely.
ACKs for top commit:
laanwj:
re-ACK 916d3596c493fec44da86aeb92b61eafeea0b596
Tree-SHA512: d8eb26b570ee730fdd75ca916507134db5f2f68987a911e33544b7f1c9ccfd1c76b9c9db63056971956b6daf16910f17ecfc197481c2f7b0773afdfbf7d381cf
d3e8adfada889a3c9fba930086eda609509aca07 util: remove c-string interfaces for DecodeBase58{Check} (Sebastian Falbesoner)
Pull request description:
This micro-PR gets rid of base58 function interfaces that are redundant in terms of c-string / std::string variants; the c-string interface for `DecodeBase58Check` is completely unused outside the base58 module, while the c-string interface for `DecodeBase58` is only used in unit tests, where an implicit conversion to std::string is not problematic.
ACKs for top commit:
practicalswift:
ACK d3e8adfada889a3c9fba930086eda609509aca07 -- patch looks correct
laanwj:
Code review ACK d3e8adfada889a3c9fba930086eda609509aca07
Tree-SHA512: 006a4a1e23b11385f60820c188b8e6b1634a182ca36e29a6580f72150214c65a3fdb273ec439165f26ba88a42d2bf5bab1cf3666a9eaee222fb4e1c00aeba433
1ccb9f30c040daf688f89f0d63e9f5e7b131d193 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr)
Pull request description:
#9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else.
So rather than just lose it, might as well get it merged in independently.
I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.
ACKs for top commit:
fanquake:
ACK 1ccb9f30c040daf688f89f0d63e9f5e7b131d193 - checked that the binaries produced are the same.
Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
1e58bcc9afefcf009653567c6373b4f7facba8f5 wallet: Fix clang build in Mac (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Top commit has no ACKs.
Tree-SHA512: 19312929af14dab97c37cf4547fbd6589a6de960f1a499c2118bb684240639af4b127cf8dc4d201b41d253cfbb645614a0606d4ecce29f300b10c210d38a961b
It includes only this commit for sake of backporting HasWalletSpent
[RPC] bumpfee
This command allows a user to increase the fee on a wallet transaction T, creating a "bumper" transaction B.
T must signal that it is BIP-125 replaceable.
T's change output is decremented to pay the additional fee. (B will not add inputs to T.)
T cannot have any descendant transactions.
Once B bumps T, neither T nor B's outputs can be spent until either T or (more likely) B is mined.
Includes code by @jonasschnelli and @ryanofsky
9c59f9c285303659ee1beed7555bbb322e6e6981 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
ACKs for top commit:
achow101:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981
ryanofsky:
Code review ACK 9c59f9c285303659ee1beed7555bbb322e6e6981. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285303659ee1beed7555bbb322e6e6981 tested rebased on current master b33136b6ba9887f7d and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285303659ee1beed7555bbb322e6e6981
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
c8583022800410afeb75e0154df7290d080d581d Change format of log2_work for uniform output (zero-padded) (jmorgan)
Pull request description:
Motivation:
It's jarring to watch the output of `tail -f ~/btcdata/debug.log` scroll by and very frequently see columns not lining up correctly because `log2_work` somtimes has less precision than 8 digits.
Current display:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
Display with this commit:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
ACKs for top commit:
practicalswift:
ACK c8583022800410afeb75e0154df7290d080d581d -- patch looks great :)
achow101:
ACK c8583022800410afeb75e0154df7290d080d581d
laanwj:
Tested ACK c8583022800410afeb75e0154df7290d080d581d
Tree-SHA512: 16cbe419c4993ad51019c676e8ca409ef1025b803cc598437c780dd7ca003d7e4ad421f451e9a374e0070ee9b3ee601b7aba849e1f346798f9321d1bce5c4401
a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b43ed16252428e935d3aeb5a38f582e0 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d5bb5a1564c339a0e6a16bac8f02c98 refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf57ccd6cdf5ec706ac328c62142d7a2 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6d0f2543948d64ed26f7d9a903f67e5 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8bb733d7d89e36b28671cb47f436292 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
7eaf86d3bfc83f2beb3ef449707d5156853126fb trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5892842f13dee89ae31812a28ab25d1 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: a31be09bfd77eed497a8e251d31358e16e2f2eb1 and 7e89994133725125dddbfa8d45484e3b9ed51c6e 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 7eaf86d3bfc83f
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
8e08d005989c6b5f7f05e0a1e0ba84f544a76d01 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 8e08d005989c6b5f7f05e0a1e0ba84f544a76d01
promag:
ACK 8e08d005989c6b5f7f05e0a1e0ba84f544a76d01.
Tree-SHA512: 0223dddf5ba28b0bfaefeda1b03b4ff95bf7e7d0c1e7b32368171e561813e22129f2a664f09279fa3b4fa63259b7680d55aa3fe66db9c7ae0039b7f529777ec3
a9d28afe23a94efdccc53f9f10716f3a0c9337eb qt: Display warnings as rich text (Hennadii Stepanov)
Pull request description:
On master (6621be53517d69ab855cee4a5978a44d6a133ba3), warnings that contain `<hr />` HTML tag are not displayed correctly:
Fixed:
ACKs for top commit:
jonasschnelli:
utACK a9d28afe23a94efdccc53f9f10716f3a0c9337eb
promag:
Code review ACK a9d28afe23a94efdccc53f9f10716f3a0c9337eb.
Tree-SHA512: ba5b3837d5f6ea15c3255a3120c9753fc58ee67a370c388556214048ab993c45be720af7cb8d43bb0f12088956cb78abc77546ed1fc691082880438072fe774b
1a9ef1d398dd14728b6bc67a89139cdf827c9753 refactor: Replace RecursiveMutex with Mutex in Shutdown() (Hennadii Stepanov)
Pull request description:
Step by step, going to replace all of the `RecursiveMutex` instances with the `Mutex` ones throughout the code base :)
Not sure if it is possible in all cases though...
This one is a low-hanging fruit.
ACKs for top commit:
MarcoFalke:
ACK 1a9ef1d398dd14728b6bc67a89139cdf827c9753 Shutdown is not recursive, so the same thread can never lock twice (UB)
vasild:
ACK 1a9ef1d3 verified manually that `Shutdown()` is not called from places that could be called from inside `Shutdown()`.
Tree-SHA512: 362a507b1a6f97dc351f708224aedbfe4bee03c4398f394d78ee31c24d76a7012ffff0e6766866cd5fd9a8e0d8840f05a2741111fe583aa20d45f0af3df0dcfa
faca73000fa8975c28f6be8be01957c1ae94ea62 ci: Install fixed version of clang-format for linters (MarcoFalke)
fa4695da4c69646b58a8fa0b6b30146bb234deb8 build: Sort Makefile.am after renaming file (MarcoFalke)
cccc2784a3bb10fa8e43be7e68207cafb12bd915 scripted-diff: Move ui_interface to the node lib (MarcoFalke)
fa72ca6a9d90d66012765b0043fd819698b94ba8 qt: Remove unused includes (MarcoFalke)
fac96e6450d595fe67168cb7afa7692da6cc9973 wallet: Do not include server symbols (MarcoFalke)
fa0f6c58c1c6d10f04c4e65a424cc51ebca50a8c Revert "Fix link error with --enable-debug" (MarcoFalke)
Pull request description:
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols.
Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.
ACKs for top commit:
Sjors:
ACK faca730
laanwj:
ACK faca73000fa8975c28f6be8be01957c1ae94ea62
hebasto:
re-ACK faca73000fa8975c28f6be8be01957c1ae94ea62, since the [previous](https://github.com/bitcoin/bitcoin/pull/19331#pullrequestreview-434420539) review:
Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4
## Issue being fixed or feature implemented
Fix/tidy up a few `qdata`/`qwatch` related parts, improve performance
for regular non-watching nodes
~based on #5744 atm~
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
01e283068b9e6214f2d77a2f772a4244ebfe2274 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca41eebb1738fdda4451d1466e77772e [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2c8741ca9d825eaaef736ede484bc85 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2427599c129f4ff581d4d045461e459 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b671ff73f13a1b5053338b6abbdb471b5 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc40d56bb532278f16ce632c5a8b8035e [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df6296609570e368e5f326979279041c11f [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b08ac4b21b35c426bf0e1b9e7c97983b [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae7333c6700d9b737d09fae0f3f4d7fa [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e92a55925308363ccdad04dcfc820d96 [doc] Describe different connection types (Amiti Uttarwar)
442abae2bac7bff85886143df01e14215532b974 [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb05235ecb85ec9d75b09c66e71268c9889 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812ddf1d946bc5acca406a7ed2dca064a6 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2de915fc3dce37fc8fac39be1c8b6fa [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438537e192230486dffcec0228a53878d [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100201754fb32440bec3e3b78cd3f0e6d [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e95d0f8f958cb35f31c3d964c57e484d scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)
Pull request description:
**This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.
**This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.
This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
(some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)
Overview of this PR:
* rename `oneshot` to `addrfetch`
* introduce `ConnectionType` enum
* one by one, add different connection types to the enum
* expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
* fix the bug in counting different type of connections
* some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.
ACKs for top commit:
jnewbery:
utACK 01e283068b9e6214f2d77a2f772a4244ebfe2274
laanwj:
Code review ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274.
fanquake:
ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
jb55:
wow this code was messy before... ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
c57f03ce1741b38af448bec7b22ab9f8ac21f067 refactor: Replace const char* to std::string (Calvin Kim)
Pull request description:
Rationale: Addresses #19000
Some functions should be returning std::string instead of const char*.
This commit changes that.
Main benefits/reasoning:
1. The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
2. All call sites convert to string anyway
ACKs for top commit:
MarcoFalke:
re-ACK c57f03ce17 (no changes since previous review) 🚃
Empact:
Fair enough, Code Review ACK c57f03ce17
practicalswift:
ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067 -- patch looks correct
hebasto:
re-ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067
Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd
412d5fe8791c417bf46fc55a5bb8d59be98a33db QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr)
2abe8cc3b760219cfa434e4c96e9f8d3611d0037 Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr)
Pull request description:
#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT.
Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation.
ACKs for top commit:
sipa:
ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db
jnewbery:
Tested ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db.
Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615
050e2ee6f28e7b31c167013be7313726e34084e9 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)
Pull request description:
Fix the following error in travis:
test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
const BlockValidationState state_dummy;
ACKs for top commit:
MarcoFalke:
Tested ACK 050e2ee6f28e7b31c167013be7313726e34084e9 on xenial with clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
fanquake:
ACK 050e2ee6f28e7b31c167013be7313726e34084e9 - I see why we didn't hit this on master. We are installing the `clang-8` packages for the tsan job. However on the 0.20 branch we are still just installing `clang`, which is 3.8.
Tree-SHA512: 8a1d57289dbe9895ab79f81ca87b4fd723426b8d72f3a34bec9553226fba69f6dc19551c1f1d52db6c4b2652164a02ddc60f3187c3e2ad7bcacb0aaca7fa690a
fa1f840596554ed264d11ee3b3643bf99eb57eb5 rpcwallet: Replace pwallet-> with wallet. (MarcoFalke)
fa182a8794cbf9be1aa91d1d75e65bc7800156bc rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} (MarcoFalke)
Pull request description:
Closes#18943
ACKs for top commit:
laanwj:
ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5
ryanofsky:
Code review ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5 and thanks for using a standalone commit for the fix
promag:
Code review ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5.
hebasto:
ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5, tested on Linux Mint 19.3.
Tree-SHA512: 0838485d1f93f737ce5bf12740669dcafeebb78dbc3fa15dbcc511edce64bf024f60f0497a04149a1e799d893d57b0c9ffe442020c1b9cfc3c69db731f50e712
f5a3a5b9ab362c58fa424261f313aa9cf46d2a98 gui: Add close window shortcut (Miguel Herranz)
Pull request description:
CMD+W is the standard shortcut in macOS to close a window without
exiting the program.
This adds support to use the shortcut in both main and debug windows.
ACKs for top commit:
jonasschnelli:
Tested ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98
hebasto:
ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98, tested on Linux Mint 19.3 by manually opening available dialogs and sub-windows, and applying the `Ctrl+W` shortcut. Also tested with "Minimize on close" option enabled / disabled.
Tree-SHA512: 39851f6680cf97c334d5759c6f8597cb45685359417493ff8b0566672edbd32303fa15ac4260ec8ab5ea1458a600a329153014f25609e1db9cf399aa851ae2f9
8d75115844baefe5cad4d82ec8dce001dd16eb9c qt: Add privacy feature to Overview page (Hennadii Stepanov)
73d8ef72742ab9193e9e95158b26176bfaab3f66 qt: Add BitcoinUnits::formatWithPrivacy() function (Hennadii Stepanov)
Pull request description:
This PR allows to hide/reveal values on the Overviewpage by checking/unchecking Menu->Settings-> Mask Values
Closes#16407
Privacy mode is OFF (the default behavior):
Privacy mode is ON:
ACKs for top commit:
jonatack:
Tested ACK 8d75115
jonasschnelli:
Tested ACK 8d75115844baefe5cad4d82ec8dce001dd16eb9c
Tree-SHA512: 42f396d5bf0d343b306fb7e925f86f66b3fc3a257af370a812f4be181b5269298f9b23bd8a3ce25ab61de92908c4018d8c2dc8591d11bc58d79c4eb7206fc6ec
c4b574899abfa27f83c6948593838ed418c78f9c gui: Add Close All Wallets action (João Barbosa)
f30960adc02877267cb6efeb378962ed96628741 gui: Add closeAllWallets to WalletController (João Barbosa)
Pull request description:
This PR adds the action to close all wallets.
<img width="405" alt="Screenshot 2020-06-01 at 01 06 12" src="https://user-images.githubusercontent.com/3534524/83365986-25a8b980-a3a4-11ea-9613-24dcd8eaa55c.png">
ACKs for top commit:
jonasschnelli:
Tested ACK c4b574899abfa27f83c6948593838ed418c78f9c
Tree-SHA512: 049ad77ac79949fb55f6bde47b583fbf946f4bfaf3d56d768e85f813d814cff0fe326b700f7b5e383cda4af7b5666e13043a6aaeee3798a69fc94385d88ce809
a30b0a24e97eae7f6d1428c5bf339a579872f28e build: enable -Werror=gnu (Vasil Dimov)
Pull request description:
Stop the build if a warning is emitted due to `-Wgnu` and
`--enable-werror` has been used. As usual - this would help notice such
a warning that is about to be introduced in new code.
This is a followup to
https://github.com/bitcoin/bitcoin/pull/18088 build: ensure we aren't using GNU extensions
ACKs for top commit:
practicalswift:
ACK a30b0a24e97eae7f6d1428c5bf339a579872f28e
Empact:
ACK a30b0a24e9
Tree-SHA512: f81b71cf3ee4db88b6f664c571075e0d30800a604f067f44273f256695a1dea533779db2ac859dd0a4cd8b66289c3e45f4aff1cfadfa160a1c354237167b05e2
68537275bd91d1dc14a69609ae443f955bfdbd64 build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d38cfd4eb7204ecd327df213958e51a refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30678ac9b8338ea920e50b7296da6bd5 test: Fix outstanding -Wsign-compare errors (Ben Woosley)
Pull request description:
Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545
ACKs for top commit:
fjahr:
re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
practicalswift:
ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
## Issue being fixed or feature implemented
Asset Unlock tx uses platform's quorum on devnets, testnet, mainnet, but
still quorum type "Test (100)" on Reg Tests
That's part II PR, prior work is here:
https://github.com/dashpay/dash/pull/5618
## What was done?
- Removed `consensus.llmqTypeAssetLocks` which has been kept only for
RegTest - use `consensus.llmqTypePlatform` instead.
- Functional test `feature_asset_locks.py` uses `llmq_type_test = 106`
instead `llmq_type_test = 100` for asset unlock tx
- there's 4 MNs + 3 evo nodes instead 3 MNs as before: evo nodes
requires to have IS to be active
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
Asset Unlock tx uses correct quorum "106 llmq_test_platform" on reg test
instead "100 llmq_test"
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
we should not vote on triggers from the past
## What was done?
## How Has This Been Tested?
n/a
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
07ddecb84e6097684fa56cfc79c8c2aad76f6604 refactor: Use [[maybe_unused]] attribute (Hennadii Stepanov)
55e0fc8df9c4045453982888732a0dd7c99ea6d1 refactor: Drop unneeded workarounds aimed to silence unused warning (Hennadii Stepanov)
Pull request description:
This change is required for bitcoin/bitcoin#24773 as it prevents MSVC yelling about "warning C4551: function call missing argument list".
But it is useful by itself as it makes code more concise and readable.
ACKs for top commit:
Empact:
Code review ACK 07ddecb84e6097684fa56cfc79c8c2aad76f6604
laanwj:
Code review ACK 07ddecb84e6097684fa56cfc79c8c2aad76f6604
vincenzopalazzo:
ACK 07ddecb84e
w0xlt:
ACK 07ddecb
Tree-SHA512: 01791855a9ba742202d5718203303af989fcb501b7cf2a24ac8d78e87487acca38f77bef264b8e27e41ad1ccf96e426725cf65bfd96ce2ac71c46b3792bed857
3429d67014095b42a976d95c3ef8622d5fe085e6 init: Prevent -noproxy and -proxy=0 settings from interacting with other settings (Ryan Ofsky)
Pull request description:
Prevent `-noproxy` and `-proxy=0` settings from interacting with `-listen`, `-upnp`, and `-natpmp` settings.
These settings started being handled inconsistently in the `AppInitMain` and `InitParameterInteraction` functions starting in commit baf05075fa from #6272:
baf05075fa/src/init.cpp (L990-L991)baf05075fa/src/init.cpp (L687)
This commit changes both functions to handle proxy arguments the same way so
there are not side effects from specifying a proxy=0 setting.
This change was originally part of #24830 but really is independent and makes more sense as a separate PR
ACKs for top commit:
hebasto:
ACK 3429d67014095b42a976d95c3ef8622d5fe085e6, tested on Ubuntu 22.04.
Tree-SHA512: c4c6b4aeb3c07321700e974c16fd47a1bd3d469f273a6b308a69638db81c88c4e67208fddc96fcda9c8bd85f3ae22c98ca131c9622895edaa34eb65c194f35db
c71117fcb04fc2e45b5e76fe96b077a07b0c0f82 net: remove non-blocking bool from interface (Bushstar)
Pull request description:
SetSocketNonBlocking was added in 0.11 in the PR below with a second argument to toggle non-blocking mode for the socket. That argument has always been set to true in all subsequent releases and I'm not sure why it is present.
https://github.com/bitcoin/bitcoin/pull/4491
ACKs for top commit:
promag:
Code review ACK c71117fcb04fc2e45b5e76fe96b077a07b0c0f82.
lsilva01:
Code review ACK c71117fcb0
vasild:
ACK c71117fcb04fc2e45b5e76fe96b077a07b0c0f82
Tree-SHA512: feebfcfa75d997460a0ba42ffe1e0c25a7e0bfcad12510ad73ea4942cc1c653f9ad429adbbb00b9288fe319009552906fcb635a14dfd7dcbde3853baab6be065
a4f4f89815c5aadff51a7a11e0d63caf5212345a Replace uint256 specific implementations of base_uint::GetHex() and base_uint::SetHex() with proper ones that don't depend on uint256 and replace template methods instantiations of base_uint with template class instantiation (Samer Afach)
Pull request description:
The current implementations of `SetHex()` and `GetHex()` in `base_uint` use `arith_uint256`'s implementations. Which means, any attempt to create anything other than `arith_uint256` (say `arith_uint512`) and using any of these functions (which is what I needed in my application) will just not work and will cause compilation errors (besides the immediate linking errors due to templates being in source files instantiated only for 256) because there's no viable conversion from `arith_uint256` and any of the other possible types. Besides that these function will yield wrong results even if the conversion is possible depending on the size. This is fixed in this PR.
ACKs for top commit:
laanwj:
re-ACK a4f4f89815c5aadff51a7a11e0d63caf5212345a
Tree-SHA512: 92a930fb7ddec5a5565deae2386f7d2d84645f9e8532f8d0c0178367ae081019b32eedcb59cc11028bac0cb15d9883228e016a466b1ee8fc3c6377b4df1d4180
b72925e7cea11522aca65580c136dbacb2753e83 lint: remove qt SIGNAL/SLOT lint (fanquake)
Pull request description:
I think we are past the point where we need to lint for this, the CPU
can probably be better utilized.
ACKs for top commit:
laanwj:
ACK b72925e7cea11522aca65580c136dbacb2753e83
Tree-SHA512: 3da6e4811cdd16ff64c7e26f641f7b24f0405cc86cec36666de58691d447eca8662c924df31c6c60b3523c13590bdc62205a3237b1b1794dd8cdef35519309b3
112a7ab9a8e4c96f5750ac3b929b433d8507354c refactor: remove macOS MAP_ANONYMOUS work around (fanquake)
Pull request description:
This was added to support compilation on macOS 10.10, our minimum
required macOS is now 10.15. macOS has also supported it since 10.11.
See https://github.com/bitcoin/bitcoin/pull/9063.
macOS 12.3 manpage for mmap:
```bash
MAP_ANONYMOUS Synonym for MAP_ANON.
MAP_ANON Map anonymous memory not associated with any specific file.
```
ACKs for top commit:
laanwj:
Code review ACK 112a7ab9a8e4c96f5750ac3b929b433d8507354c
jarolrod:
ACK 112a7ab9a8e4c96f5750ac3b929b433d8507354c
Tree-SHA512: 920744c755d05d813ab312ff27e42eacb27b1297972800e6fb64bbaad1ea14258751a7dd80c07bfa554a172f36960b26a07505f67e82885253c8bf551073c38e
e8fc236da70085d30c90cdade06edfa1da855a6c refactor: add missing std:: includes to threadnames.cpp (fanquake)
87f3c04cc539c34d32af5fd59abef2c0b5faee26 doc: remove incorrect mention of PR_GET_NAME (fanquake)
Pull request description:
By removing the whole comment. These `#include // For` comments are near impossible
to maintain, pollute diffs, and generally don't add a lot of value.
While here, also add the missing `std::` includes.
ACKs for top commit:
junderw:
LGTM ACK e8fc236
Tree-SHA512: d29aff40c94f59c42f295a5738bc5ff2f4a2f2e6d270cc505f27d56d07d272597e2f8403d72fe45775661e1a1fc2af9fc52aeaeb41263bd3e9dfe255332383c8
## Issue being fixed or feature implemented
Dead-code, useless conditions can be potential source of bug.
## What was done?
See each particular commit.
This particular commit "fix: check ptr in assert before usage" fixes
potential UB - `assert` is better than UB.
All other commits are not fixing any real issue, just to tidy-up code a
bit or to shut a potential warning.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
f8cba0d9117fe9b9ac51d7044372b28270c7838b test: Change default test logging directory (Yancy Ribbens)
Pull request description:
This PR changes the default test log location request here: https://github.com/bitcoin/bitcoin/issues/17224. Instead of using the location of the makefile [automatic variable](https://www.gnu.org/software/make/manual/make.html#Automatic-Variables) `$<` I extract just the basename and then prepend a new location `./test`. This is done because `$<` represents the variable name AND location of the prerequisite here.
Top commit has no ACKs.
Tree-SHA512: f0fbc530cf0e14c284b4bbf6671c145b1d7a2e1f5561f5c5d09f0cbe88b98e620e763bbbf2dfa9aeeec3dcc9b0127939e105e14c7e4f6660c7c19663622a393d
e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov)
Pull request description:
bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly.
This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again.
Fixes bitcoin-core/gui#545.
ACKs for top commit:
RandyMcMillan:
tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3
Sjors:
tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 (rebased on master) indeed fixes the crash described in #545
promag:
Tested ACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 on macOS 10.15 with Qt 5.15.2.
Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131
faa7d8a3f7cba02eca7e247108a6b98ea9daf373 util: Add SaturatingAdd helper (MarcoFalke)
Pull request description:
Seems good to have this in the repo, as it might be needed to write cleaner code. For example:
* https://github.com/bitcoin/bitcoin/pull/24090#issuecomment-1019948200
* https://github.com/bitcoin/bitcoin/pull/23418#discussion_r744953272
* ...
ACKs for top commit:
MarcoFalke:
Added a test. Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa90189cbf faa7d8a3f7`
klementtan:
reACK faa7d8a3f7
vasild:
ACK faa7d8a3f7cba02eca7e247108a6b98ea9daf373
Tree-SHA512: d0e6efdba7dfcbdd16ab4539a7f5e45a97d17792e42586c3c52caaae3fc70612dc9e364359658de5de5718fb8c2a765a59ceb2230098355394fa067a9732bc2a
71d33380ed6858b4a65b396332bfb22d984642a6 qt: prevent negative values of progressPerHour (HiLivin)
Pull request description:
Added a similar guard to _progressPerHour_ as is placed at _remainingMSecs_.
It prevents the display of negative values like "-0.00%" in some cases.
ACKs for top commit:
hebasto:
ACK 71d33380ed6858b4a65b396332bfb22d984642a6
jarolrod:
ACK 71d3338
shaavan:
reACK 71d33380ed6858b4a65b396332bfb22d984642a6
Tree-SHA512: 5427cdf4441b542196008034355ea00a075adf8b9aeeb383bacdb4e5fbda23d665448a50035aac93cbf401d5d6211d39a2c7c294568d9f5548a5c7579e201c44
faa5e171e6bdb8f3b4027a3f06497f0de5abf766 RPCConsole: Throw when overflowing size_t type for array indices (MarcoFalke)
Pull request description:
To test:
-> `getblock(getbestblockhash(), 1)[tx][22222222222222222222222222222]`
Before:
<- `868693731dea69a197c13c2cfaa41c9f78fcdeb8ab8e9f8cdf2c9025147ee7d1` (hash of the coinbase tx)
After:
<- `Error: Invalid result query`
ACKs for top commit:
jarolrod:
ACK faa5e171e6bdb8f3b4027a3f06497f0de5abf766
shaavan:
ACK faa5e17
Tree-SHA512: ddff39aae1c15db45928b110a9f1c42eadc5404cdfa89d67ccffc4c6af24091967d43c068ce9e0c1b863cfc4eb5b4f12373a73756a9474f8294e8a44aabc28d8
9bd168bf5457c6fd9770769547d8757bf14813b0 qt: add missing tooltips to options menu settings (Jarol Rodriguez)
Pull request description:
This adds missing tooltips to the text of the `Size of database cache` and the `Number of script verification threads` settings.
All settings in the Options window will now have appropriate tooltip texts.
ACKs for top commit:
jonatack:
ACK 9bd168bf5457c6fd9770769547d8757bf14813b0 tested on Debian 5.10.46-4 (2021-08-03)
hebasto:
ACK 9bd168bf5457c6fd9770769547d8757bf14813b0, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: d71946bfee33c624a8b79eafe514d2c902090a40bc25097be4c7da4a80270f53305002af1b27d5fd082a0f45f838e22036632f9445918c4b8898073b33c09c08
93cc53a2b27eeb05fe00b8bf17465037815473a1 gui: Unregister wallet notifications before unloading wallets (Russell Yanofsky)
Pull request description:
This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.
ACKs for top commit:
promag:
Code review ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1.
hebasto:
ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1
Tree-SHA512: 805f50a493291ad0f7c48725fbc5058d58ebbdb0770befd51d8aa241209a13f8a46f5982481336ab8338cdc83e9017668089a71deccf1587308e841cf8697825
87651795d8622d354f8e3c481eb868d9433b841c fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517f3418e2a408071b4c2ce26571f3167 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)
Pull request description:
Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
ACKs for top commit:
practicalswift:
cr ACK 87651795d8622d354f8e3c481eb868d9433b841c
jonatack:
ACK 87651795d8622d354f8e3c481eb868d9433b841c rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
fadd73037e266edb844f0972e82e9213171ef214 refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e266edb844f0972e82e9213171ef214
theStack:
Code-review ACK fadd73037e266edb844f0972e82e9213171ef214
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
33c6a208a9e2512a174c99c224a933a59f091bc2 span, doc: provide span.h context and explain lifetimebound definition (Jon Atack)
d14395bc5db55331115fa3c1e71741d1de7f092f net, doc: provide context for UnserializeV1Array() (Jon Atack)
Pull request description:
Add contextual documentation for developers and future readers of the code regarding
- CNetAddr::UnserializeV1Array (see #22140)
- Span and why it defines Clang lifetimebound locally rather than using the one in attributes.h
ACKs for top commit:
laanwj:
Documentation review ACK 33c6a208a9e2512a174c99c224a933a59f091bc2
Tree-SHA512: cb8e6a6c23b36c9ef7499257e97c5378ec895bb9122b79b63b572d9721a1ae6ce6c0be7ad61bdf976c255527ae750fc9ff4b3e03c07c6c797d14dbc82ea9fb3a
ef72e9bd4124645fe2d00521a71c1c298d760225 doc: nChainTx needs to become a 64-bit earlier due to SegWit (Sjors Provoost)
Pull request description:
As of block 597,379 txcount is 460,596,047 (see `chainparams.cpp`), while `uint32` can handle up to 4,294,967,296.
Pre segwit the [minimum transaction size](https://en.bitcoin.it/wiki/Maximum_transaction_rate) was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for `unsigned int nChainTx` says, that should last until the year 2030.
With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see #15482), without a witness:
```
4 bytes version
1 byte input count
36 bytes outpoint
1 byte scriptSigLen (0x00)
0 bytes scriptSig
4 bytes sequence
1 byte output count
8 bytes value
1 byte scriptPubKeyLen
1 byte scriptPubKey (OP_TRUE)
4 bytes locktime
```
That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024.
Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it.
ACKs for top commit:
practicalswift:
re-ACK ef72e9bd4124645fe2d00521a71c1c298d760225
jarolrod:
ACK ef72e9bd4124645fe2d00521a71c1c298d760225
theStack:
ACK ef72e9bd4124645fe2d00521a71c1c298d760225
Tree-SHA512: d8509ba7641796cd82af156354ff3a12ff7ec0f7b11215edff6696e95f8ca0e3596f719f3492ac3acb4b0884ac4e5bddc76f107b656bc2ed95a8ef1b2b5d4f71
350e034e64d175f3db4c85ddca42e76e279912f6 consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack)
Pull request description:
Commit ccd8ef65 "Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock" in #11281 moved the cs_main lock from caller to `ReadBlockFromDisk()` for calling `CBlockIndex::GetBlockPos()`, but the second invocation doesn't have the lock, and IIUC there is no guarantee the compiler can know if state has changed.
Use the `blockPos` local variable instead, rename it to `block_pos`, and make it const.
ACKs for top commit:
laanwj:
Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6
theStack:
Code-review ACK 350e034e64d175f3db4c85ddca42e76e279912f6
promag:
Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6.
Tree-SHA512: 0df0614ab1876885c85f7b53c604a759a29008da8027e95503b4726d2b820ec6d27546020c613337ff954406e01cb5d191978ba4a12124052fed6e1b0e9a226f
317442525586ba9ff8b9af6c506b48f87cd8cd87 Cleanup headers after #20788 (Hennadii Stepanov)
Pull request description:
This is a header cleanup after #20788.
ACKs for top commit:
vasild:
ACK 317442525586ba9ff8b9af6c506b48f87cd8cd87
Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
56a42f10f452f0ac0e3e333646a8effcbebf6b30 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)
Pull request description:
This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed https://github.com/bitcoin/bips/pull/921 BIP32 test vector 5.
ACKs for top commit:
darosior:
utACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30 -- Had to implement essentially the same fix in python-bip32.
kristapsk:
ACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30. Checked that test vectors are the same as in BIP32 and that tests pass.
Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
fabed982ad9143cddaca8346f6b4c243dd84e0c4 fuzz: Re-enable assert in banman again (MarcoFalke)
Pull request description:
Looks like this was accidentally fixed by removing the buggy code in commit efd6f904c78769ad2e93c1f1de43014d284e7561
ACKs for top commit:
hebasto:
ACK fabed982ad9143cddaca8346f6b4c243dd84e0c4
Tree-SHA512: 2dea5dad48ff2050ae7086c1c6306c40f650e9629918e79adc54164a375d777a70b29f5a480566dc6430f07ce33dfe703fc5d45a20125584b4a026c5832198a2
4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/22383#discussion_r675069128)):
b620b2d58a/src/txmempool.h (L554-L558)
`CTxMemPool::get()` acquires this lock:
b620b2d58a/src/txmempool.cpp (L822-L829)
so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked.
ACKs for top commit:
tryphe:
Concept ACK. tested 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f but not extensively.
jnewbery:
Code review ACK 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f
Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c
7b3a20b2602f902c344a615f23f8f0280b6f6bcc mempool: apply rule of 5 to epochguard.h, fix compiler warnings (Jon Atack)
Pull request description:
Apply the rule of five to `src/util/epochguard.h::{Epoch, Marker}` for safety, which also nicely fixes the `-Wdeprecated-copy` compiler warnings with Clang 13.
References:
- https://en.cppreference.com/w/cpp/language/rule_of_three
- https://www.stroustrup.com/C++11FAQ.html#default
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five
<details><summary>Compiler warnings fixed</summary><p>
```bash
In file included from policy/rbf.cpp:5:
In file included from ./policy/rbf.h:8:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
```
```bash
In file included from txmempool.cpp:6:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
node_alloc_traits::construct(
^
/usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here
final().construct_value(x,v);
^
/usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
return insert_(v,detail::lvalue_tag());
^
/usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here
{return final().insert_(x);}
^
/usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>::final_insert_' requested here
std::pair<final_node_type*,bool> p=this->final_insert_(x);
^
txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here
indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
```
```bash
In file included from test/fuzz/policy_estimator.cpp:9:
In file included from ./test/fuzz/util.h:27:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
_Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::emplace_back<CTxMemPoolEntry>' requested here
{ emplace_back(std::move(__x)); }
^
test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
```
</p></details>
ACKs for top commit:
laanwj:
Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
vasild:
ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
Tree-SHA512: 0406dfcec180152d4f9ed07cbc2f406ad739b41f9c9cb38f8c75159c15d9d8a9a5c7526765966e40d695d265c178f6a80152e7edf82da344a65e55938dddb63d
9b85a5e2f7e003ca8621feaac9bdd304d19081b4 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511d8c43b2025a89bcd8295de755346a7 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4 🎰
ryanofsky:
Code review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
## Issue being fixed or feature implemented
Platform (in the scope of Withdrawals) need to be aware if a tx isn't in
mempool when requesting status of a tx using RPC `gettxchainlocks`.
cc @markin-io
## What was done?
- mempool is passed to `GetTransaction` and saving the result for
checking latter.
- If the returned tx_ref is nullptr, then the RPC returns null for the
corresponding tx in the array.
Example:
`tx1` is mined and chainlocked, `tx2` is in mempool and `tx3` doesn't
exist.
The result now is:
`[
{
"height": 830,
"chainlock": false,
"mempool": true
},
{
"height": -1,
"chainlock": false,
"mempool": true
},
{
"height": -1,
"chainlock": false,
"mempool": false
}
]`
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Platform in the scope of credit withdrawals, need a way to get the
status of an Asset Unlock by index.
## What was done?
A new RPC was created `getassetunlockchainlocks` that accepts Asset
Unlock indexes array as parameter and return corresponding status for
each index.
The possible outcomes per each index are:
- `chainlocked`: If the Asset Unlock index is mined on a Chainlocked
block.
- `mined`: If no Chainlock information is available, and the Asset
Unlock index is mined.
- `mempooled`: If the Asset Unlock index is in the mempool.
- `unknown`: If none of the above are valid.
Note: This RPC is whitelisted for the Platform RPC user.
## How Has This Been Tested?
Inserted on `feature_asset_locks.py` covering cases where Asset Unlock
txs are in mempool, mined and not present.
## Breaking Changes
no
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
## Issue being fixed or feature implemented
Cache population for old quorums is a cpu heavy operation and should be
avoided for inactive quorums _at least_ oin `ScanQuorums`. This issue is
critical for testnet and other small network because every mn
participate in almost every platform quorum and cache population for 2
months of quorums can easily block everything for 15+ minutes on a 4 cpu
node. On mainnet quorum distribution is much better but it's still a
small waste of cpu (or not so small for unlucky nodes).
#5761 follow-up
## What was done?
Do not start cache population for outdated quorums, improve logs in
`StartCachePopulatorThread` to make it easier to see what's going on.
## How Has This Been Tested?
run a mn on testnet
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
Impossible to drop it completelly right now because:
- net doesn't know any details about chain - can't check status of fork
- the functional test feature_maxuploadtarget.py assume block size 1Mb
- DIP0001 can't be activated from regtest early block2 because big txes are
not allowed after DIP0001
refactor: drop global variable fDIP0001ActiveAtTip - attempt 2
