4e0613369f446b0a57783bf9e7258fec6c474981 qt: Elide long strings in their middle in the Peers tab (Hennadii Stepanov)
Pull request description:
The eliding of long addresses (Onion v3 etc) in the Peers tab in their middle was [discussed](https://github.com/bitcoin-core/gui/issues/262#issuecomment-810490396) in #262.
On master (f0fa32450ec35056b3e1025262aeaef4a24c35ee):
With this PR:
This PR suggests the minimal diff to achieve the goal. OTOH, this change in behavior is common for all columns in the Peers table, but it seems harmless.
ACKs for top commit:
jarolrod:
tACK 4e0613369f446b0a57783bf9e7258fec6c474981
promag:
Code review ACK 4e0613369f446b0a57783bf9e7258fec6c474981.
Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb
b109bde46a8730afbc09c107b802a2c321293f4b [test] check that mapFlagNames is up to date (glozow)
5d3ced72f9b5f36db1a76bd8bc918d11b87dfd72 [test] remove unnecessary OP_1s from invalid tests (glozow)
5aee73d1759bcc0d1e951776942e616843934af1 [test] minor improvements / followups (glozow)
8a365df5586b36d1772c78069f9d93c56a81df6f [test] fix bug in ExcludeIndividualFlags (glozow)
8cac2923f57ac33848ff41b74c3be520b75936df [test] remove invalid test from tx_valid.json (glozow)
Pull request description:
This is a followup to #19698.
- There was a bug in the `ExcludeIndividualFlags` function which is fixed here.
- Fixing this bug also showed that there is a test that's supposed to fail (already existing in tx_invalid.json) in tx_valid.json, so I removed it. Other than that, the tests should all pass.
- Also implements a few suggestions I received offline: removing the `OP_1`s from the invalid tests (similar to 19db590d04), comments, and style.
- A few other small fixes, like adding asserts, putting all the flags in `mapFlagNames`, better error messages
ACKs for top commit:
jnewbery:
utACK b109bde46a8730afbc09c107b802a2c321293f4b
Tree-SHA512: 7233a8c0f1ae1172fac8000ea6e05384ecf79074c39948d118464868505c7f02f17e96503c81bd05c07adb2087648a5d93d9899e16fdefa6b7efcb51319444a9
9053b88b1c15f57cdcff2fc1c761efebb2ebfefe update docstring in feature_csv_activation.py (Pierre K)
Pull request description:
These changes in the test documentation reflect the changes introduced in #17921.
ACKs for top commit:
MarcoFalke:
review ACK 9053b88
Tree-SHA512: 17fb954baded8dab1c869dd48b76b516150bae616c792c573e4114d4adfdd40195745c56570aa3050cc0015ee496acd7ec178df8ba14831dd22f9722fda84da2
bee56c78e94417f89b1f48682404e2821b57bdec rpc: Check default value type againts argument type (João Barbosa)
f81ef4303e057e85aa24772c865287c17ffa4350 rpc: Keep default argument value in correct type (João Barbosa)
Pull request description:
Store default values of RPC arguments in the corresponding type instead of a string. The value is then serialized when the help output is needed. This change simplifies #20017.
The following examples illustrates how to use the new `RPCArg::Default` and `RPCArg::DefaultHint`:
```diff
- {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"}
+ {"verbose", RPCArg::Type::BOOL, RPCArg::Default(false), "True for a json object, false for array of transaction ids"}
```
```diff
- {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"}
+ {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint("one month"), "Size of the window in number of blocks"}
```
No behavior change is expected.
ACKs for top commit:
LarryRuane:
ACK bee56c78e94417f89b1f48682404e2821b57bdec
MarcoFalke:
ACK bee56c78e94417f89b1f48682404e2821b57bdec 🦅
Tree-SHA512: c47d78c918e996d36631d4ad3c933b270a34c5b446b8d736be94cf4a0a7b8c0e33d954149ec786cf9550639865b79deb6a130ad044de6030f95aac33f524293a
9a0653553a0ec403b4e7c6713466e0c7fa10ec94 Refactor ProcessNewBlock to reduce code duplication (R E Broadley)
Pull request description:
There are probably a few issues with this code (maybe there's even a reason this code is duplicated as it currently is), so apologies in advance that I'm still a little (maybe very) bad with C++
ACKs for top commit:
MarcoFalke:
ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94 💻
promag:
Code review ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94.
theStack:
Code-review ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94 🌴
Tree-SHA512: f8634ffad4b2370204d1a0945db4e27248b9e579d9912784da432b8ee3303cae424fa9f7500000dcfb31e6d29d04a8f7d322d17a6fe3d4adaddd10c539458a8c
55d85834ccd73aa2f93cf9a81523cb747973346e script: Add trusted key for hebasto (Hennadii Stepanov)
Pull request description:
It is assumed that my responsibility will be limited to the [GUI repo](https://github.com/bitcoin-core/gui).
ACKs for top commit:
laanwj:
ACK 55d85834ccd73aa2f93cf9a81523cb747973346e
MarcoFalke:
matches the key I have locally ACK 55d85834ccd73aa2f93cf9a81523cb747973346e 🍪
jarolrod:
ACK 55d85834ccd73aa2f93cf9a81523cb747973346e 🥃
Tree-SHA512: 256d03e108c9a14e251340ac6e91234d076778cb6bd551439182176207051f4efc55d396754867e5a7191c8c698610f92016668e163037c67dde56f4136026b8
NetPermissions::ClearFlag() is currently only called in the codebase with
an `f` value of NetPermissionFlags::PF_ISIMPLICIT.
If that should change in the future, ClearFlag() should not be called
with `f` being a subflag of a multiflag, e.g. NetPermissionFlags::PF_RELAY
or NetPermissionFlags::PF_DOWNLOAD, as that would leave `flags` in an
invalid state corresponding to none of the existing NetPermissionFlags.
Therefore, allow only calling ClearFlag with the implicit flag for now.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
to clarify/test the relationship and NetPermissions operations
involving the NetPermissionFlags PF_NOBAN and PF_DOWNLOAD.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
586190f0b4740457cb86cba632e3d64e6dfe9b0c rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd369027273278a0541f3b991eb71de831aa2 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f92b8b9cef1b83f5ee8e6a9267b4af0 rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d1920209cded0dae52fb9070a3530d9a4e5fd rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e3511e8bb6e163305cab0a96783d25b rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4d393b091479be5a5df5a0a160cf986 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98612366df031bebef9e0468fb57b8a2 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3eeb2502904ed4698646d2c86d028aad2 rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9b18fe8f151771a83ccae1681f16c66 rest: Use existing NodeContext (Carl Dong)
3f0893479908ca28d6127c8d0ada30737cb830be rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b950842fc3a17641a4a21501de0a800b5 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6f6388c6f86729a0edd7cb69f1748e0 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2b62dc74a485569d08130dc3ef9ff63 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf429586e3a5b4c3231fe430dc29695481 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9133e0fc6211e987421ad1d9be1a399 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
- [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
ryanofsky:
Code review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates!
jnewbery:
utACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c
MarcoFalke:
review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c 🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
7f3a5980c1d54988a707b961fd2ef647cebb4c5b qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)
Pull request description:
Windows and macOS do [not support](https://doc.qt.io/qt-5/qclipboard.html#notes-for-windows-and-macos-users) the global mouse selection.
Fixes #258.
ACKs for top commit:
promag:
Code review ACK 7f3a5980c1d54988a707b961fd2ef647cebb4c5b.
jarolrod:
ACK 7f3a5980c1d54988a707b961fd2ef647cebb4c5b
Tree-SHA512: be2beeef7d25af6f4d4a4548325d8d29f08e4342f499666bc4a670ed468a63195d514077c2cd0dba197e12bd43316fd3e2813cdc0954364b6aa4ae6b90c118bf
63631beef6a0046390469971adf4500718ab34ad test: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic` (practicalswift)
Pull request description:
Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic`.
Fixes #21682.
Rationale from https://github.com/bitcoin/bitcoin/issues/21682#issuecomment-819897122:
> I've looked at that test before and I don't think that specific `BOOST_CHECK` makes much sense TBH :)
>
> 1.) I don't understand why we test if `ToString()` output includes `%zone_index`: it clearly doesn't on some platforms, so we cannot rely on it anyways. Then why test it?
>
> 2.) And perhaps more fundamentally: why would we even _want_ to have `%zone_index` in our textual `ToString()` output? I think the expectation is to get say `fe80::1ff:fe23:4567:890a` (without zone index) and not say `fe80::1ff:fe23:4567:890a%eth2 ` or `fe80::1ff:fe23:4567:890a%3 `when doing `ipv6_addr.ToString()` :)
ACKs for top commit:
MarcoFalke:
review ACK 63631beef6a0046390469971adf4500718ab34ad
Tree-SHA512: 06863d1edfb9ad1ca9bcae09cf3f0f47b58bb29d222b70799c3dc059b96452889026e4b99b132782846d9896e3e798d17c7f9406e0e6a0bec1bffc6edb54e9df
44dab423eb88dbf854d22f2991e79c828ffac0f2 qa: Test default include_mempool value of gettxout (João Barbosa)
Pull request description:
With the following diff the functional test would pass. Fix by testing the default value.
```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1142,7 +1142,7 @@ static RPCHelpMan gettxout()
uint256 hash(ParseHashV(request.params[0], "txid"));
int n = request.params[1].get_int();
COutPoint out(hash, n);
- bool fMempool = true;
+ bool fMempool = false;
if (!request.params[2].isNull())
fMempool = request.params[2].get_bool();
```
ACKs for top commit:
MarcoFalke:
cr ACK 44dab423eb88dbf854d22f2991e79c828ffac0f2
Tree-SHA512: 14db21b29d6b2c01d1d1278e18a0cf35d6ae566e33e45515d1fe2983dda94ad1ff6065c217601d283f9515cae39b57e981b62ac71ec2002de5359bd8a9e3efa9
5f2be6e71e6130b58ebfbf81aaf48ce90dd9d179 Remove no longer used contrib/bitcoin-qt.pro from the repo (Hennadii Stepanov)
Pull request description:
From [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2021-04-15.html#l-209):
> \<hebasto> wumpus: I cannot see any way how the `contrib/bitcoin-qt.pro` is used in the translation process, neither in the main repo nor in https://github.com/bitcoin-core/bitcoin-maintainer-tools. Besides it looks outdated and unmaintained. May I ask you to confirm/deny my assumption?
> \<wumpus> hebasto: it is not used for anything, it exists to be able to edit the qt forms in qt designer nothing more
> \<wumpus> i'm not sure if it is even *necessary* for that, but it is why it is there
> \<hebasto> wumpus: thanks, qt designer does not need *.pro file at all
> \<hebasto> maybe qt creator does
> \<wumpus> feel free to create a PR to remove it, best way to find out if someone wants to keep it, you are right it hasn't been updated in a long time
> \<hebasto> ok
> \<wumpus> fwiw, the only question i get about it ever is why it exists
> \<hebasto> it was in use with `qmake` years ago (what I found digging into the repo history)
> \<wumpus> yes, that was the original reason, but when we switched to automake it was kept around for use w/ qt's GUI tools
> \<hebasto> I've noticed it in https://github.com/bitcoin/bitcoin/blame/master/doc/translation_process.md#L25
> \<wumpus> what it says there is definitely not true anymore
ACKs for top commit:
laanwj:
ACK 5f2be6e71e6130b58ebfbf81aaf48ce90dd9d179
jarolrod:
ACK 5f2be6e71e6130b58ebfbf81aaf48ce90dd9d179
Tree-SHA512: 7c105612f28185097fee9e4108b162b4c8b07cc527f4438bdf5bcab08c65421ea301de8584d58770cd113fa871f6781daa8145bd6463278523449e28bfc49d06
549c82ad3a34a885ecca37a5f04c36dfbaa95d17 fuzz: use ConsumeBool() instead of !ConsumeBool() (Vasil Dimov)
29ae1c13a59187119f5b2a38b54dbbec936d8f87 fuzz: split FuzzedSock interface and implementation (Vasil Dimov)
9668e43d8e757c0185b900eb6ee6891a0ba41666 fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event (Vasil Dimov)
0c90ff1429deaa556c0509c13cdd5aef5df9c0d4 fuzz: set errno from FuzzedSock::Wait() if it simulates a failure (Vasil Dimov)
5198a02de4e7a1b0efe28c6095745ce59f7f98c4 style: remove extra white space (Vasil Dimov)
Pull request description:
* split FuzzedSock interface and implementation
* make FuzzedSock::Wait() sometimes simulate an occurred event
* set errno from FuzzedSock::Wait() if it simulates a failure
(this is a followup from https://github.com/bitcoin/bitcoin/pull/21617)
ACKs for top commit:
practicalswift:
cr ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17: patch looks correct and touches only `src/test/fuzz/`
MarcoFalke:
re-ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17 only change is rebase 🎬
Tree-SHA512: 8ba965a8319074ad2ef840219c35c77e37cc79f00fb3926f20ccbf5f58e9616f5a3ac96434ad33996b47d292fa760d5d00a529001ac0d1d254262e5df93f616f
