I created some documentation as to how icons should be prepared, optimized, styled, contributed and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient.
The current icon set does not yet follow this policy but I will be updating them in future PRs. To see the current icon set using this policy see this figma source file..
Made a mistake with the last PR open for this #143 and had to re-open.
Concept ACK 8131d80a38a887e08f6a9fe49db46cd524499b1a
PR was moved because the original branch was deleted: Link to previous conversation
The current commit fails the lint-whitespace.sh linter with error message:
This diff appears to have added new lines with trailing whitespace.
Per the ci failure, It appears that the culprit may be on line 59.
33+### Optimizing Production Files (PNG):
34+Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples:
35+
36+- Open source tool [ImageOptim](https://imageoptim.com/api).
37+- Open source tool [Trimage](https://trimage.org/).
38+- (Advanced) Run them through Bitcoin Core's [optimize-png.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/master/optimise-pngs.py) script.
7+each icon.
8+
9+#### SVG Source File
10+SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support.
11+In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as:
12+Inkspace, Adobe Illustrator, Figma, Sketch, or Adobe XD.
Figma is non-free and definitely not preferred…
Actually, the preferred tool (rsvg-convert) isn’t even listed right now!
Figma is free, the paid version is only for a few extra features that do not pertain to SVG rendering. It is the preferred tool among designers (who I was pointing to in this doc as they are usually the ones who hand on the file to developers in a production sense). @jonatack I agree, tools come and go - it’s easy enough to google and use any vector tool you wish, will make this change.
For some reference here is some info around UX/UI tools usage - https://uxtools.co/survey-2020/.
43+Icons are to be added to Bitcoin Core, via a pull request (PR), under these circumstances:
44+
45+- An icon used in production is being replaced with a new icon.
46+- A feature is being added to the Bitcoin Core GUI that requires the addition of a new icon.
47+
48+If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography.
0If a new icon is designed for an upcoming or active PR, the icon designer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and make sure it is consistent with Bitcoin Core's iconography.
53+
54+When opening a PR that intends to add an icon, a SVG source file must be included under `src/qt/res/src`, and a optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size.
55+
56+
57+
58+## Attribution
https://cirrus-ci.com/task/5544742043779072?command=lint#L949
0## Attribution
5+## Preparing Icons
6+Both an icon source file, in Scalable Vector Graphics (SVG) format, and an optimized production file, in Portable Network Graphics (PNG) format, are required for
7+each icon.
8+
9+#### SVG Source File
10+SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support.
the comma isn’t actually needed here
0SVGs are used as source files because they can scale while retaining image quality but are not used in production due to limited application support.
6+Both an icon source file, in Scalable Vector Graphics (SVG) format, and an optimized production file, in Portable Network Graphics (PNG) format, are required for
7+each icon.
8+
9+#### SVG Source File
10+SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support.
11+In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as:
nit, the sentence can be simplified a bit here
0If different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as:
10+SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support.
11+In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as:
12+Inkspace, Adobe Illustrator, Figma, Sketch, or Adobe XD.
13+
14+#### PNG Production File
15+PNGs are used in production due to wide application support, transparency support, and better image quality in comparison to competing file types
nit, s/in comparison/compared
0PNGs are used in production due to wide application support, transparency support, and better image quality compared to competing file types
15+PNGs are used in production due to wide application support, transparency support, and better image quality in comparison to competing file types
16+such as JPEG.
17+
18+
19+### Icon Grid
20+Bitcoin Core uses an 8-point grid system for its icons which utilizes size increments of 8px to visually orientate elements within a grid. The majority of screen sizes are divisible by 8 which makes fitting elements easier using an 8-point grid system. This makes things more efficient for designers and maintains consistency across applications.
needs a comma, adding ‘system’ makes the antecedent clearer
0Bitcoin Core uses an 8-point grid system for its icons which utilizes size increments of 8px to visually orientate elements within a grid. The majority of screen sizes are divisible by 8, which makes fitting elements easier using an 8-point grid system. This system makes things more efficient for designers and maintains consistency across applications.
22+
23+### Icon Style
24+Icon style should adhere to the following principles to maintain consistency and clarity:
25+
26+- To ensure icons look good at any scale, a minimalistic design should be used.
27+- Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. In the event that the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system.
nit
0- Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. If the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system.
23+### Icon Style
24+Icon style should adhere to the following principles to maintain consistency and clarity:
25+
26+- To ensure icons look good at any scale, a minimalistic design should be used.
27+- Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. In the event that the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system.
28+- Icon should avoid organic shapes or elements, these do not scale well.
0- The icon should avoid organic shapes or elements; these do not scale well.
29+- Colors should be consistent with existing Bitcoin Core icons.
30+- Icons should adhere to the 8-point grid system (see above).
31+
32+
33+### Optimizing Production Files (PNG):
34+Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples:
0Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks, resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples:
45+- An icon used in production is being replaced with a new icon.
46+- A feature is being added to the Bitcoin Core GUI that requires the addition of a new icon.
47+
48+If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography.
49+
50+If a new feature which requires a new icon is being added to the Bitcoin Core GUI, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed.
0If a new feature is being added which requires a new icon, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed.
49+
50+If a new feature which requires a new icon is being added to the Bitcoin Core GUI, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed.
51+
52+Icons, both SVG and PNG, are not to be added to Bitcoin Core prior to a production use case.
53+
54+When opening a PR that intends to add an icon, a SVG source file must be included under `src/qt/res/src`, and a optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size.
0When opening a PR that intends to add an icon, an SVG source file must be included under `src/qt/res/src`, and an optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size.
faaf3954e2f0089b6c6b9965f15e7f9af09c6fb0 fuzz: Extend psbt fuzz target a bit (MarcoFalke)
Pull request description:
Previously it only merged the psbt with itself, now it tries to merge another.
ACKs for top commit:
practicalswift:
Tested ACK faaf3954e2f0089b6c6b9965f15e7f9af09c6fb0
Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
Windows and macOS do not support the global mouse selection.
8e84c1872c67b9c0e82b66146ecdb845a49aee9e Ignore guix builds (Hennadii Stepanov)
Pull request description:
This PR is a #21375 follow up.
ACKs for top commit:
sipa:
ACK 8e84c1872c67b9c0e82b66146ecdb845a49aee9e
Tree-SHA512: 71d1c5acac3382f232074a025445d6d6660cb99e53233fade9ab29ad95b56da44e4e42e44411cfef175a0a8f3800633779ad1d24c2cfdcbc1a36239d38d5b8c3
fa41a917356a7f5e0f3286b227ced7f2e6797e67 ci: Run self-hosted ci (MarcoFalke)
fa52a40f0ebb7aa33af210a18e525eec7bb6887f ci: Make cirrus cache folders relative to cirrus base dir (MarcoFalke)
fa278412a023ac150da764a465567f2970348449 ci: Restart docker before run (MarcoFalke)
fad4f48e0763d111369656dea9575a789d2016e8 ci: [refactor] Create setting for ephemeral config in .cirrus.yml (MarcoFalke)
Pull request description:
Due to our heavy use of the Cirrus CI community cluster, some tasks may take a long time to get scheduled. While it is possible to use "Compute Credits" to get immediately scheduled on the cluster, I couldn't find a sponsor that'd be willing to cover the total cost, if all tasks were paid for with credits.
However, it is also possible to bring our own runners to Cirrus CI.
For testing purposes, a single task will be transformed to run on the DrahtBot infrastructure. If all goes well, the other tasks can be moved, too.
ACKs for top commit:
hebasto:
ACK fa41a917356a7f5e0f3286b227ced7f2e6797e67, I have reviewed the code and it looks OK.
Tree-SHA512: 513738daec36da8cd48a8f11e687ff0b7dfaba1ae4ed2fa77e7b043f88fd52bf5c0dbad2768e13df88518323917f08348cb62be6376a423142921f8d2c59a938
d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec doc: release notes for new listbanned fields (Jarol Rodriguez)
60290d3f5ec8e7e3b8cb1ebae02d5d72f6005184 test: increase listbanned unit test coverage (Jon Atack)
3e978d1a5dbd43f85bd03e759984ab1f209d6e34 rpc: add time_remaining field to listbanned (Jarol Rodriguez)
5456b345312857981cb426712f0665800c682e09 rpc: add ban_duration field to listbanned (Jarol Rodriguez)
c95c61657afd058b46549fb3d65633d7c736f5fc doc: improve listbanned help (Jarol Rodriguez)
dd3c8eaa3399b28dc78a883ff78cbe7cc5c31b5b rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez)
Pull request description:
This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields
It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](https://github.com/bitcoin/bitcoin/pull/21602#issuecomment-813486134) by jonatack, `time_remaining` is another useful user-centric data point.
Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical.
This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields.
**Master: listbanned**
```
[
{
"address": "1.2.3.4/32",
"banned_until": 1617691101,
"ban_created": 1617604701
},
{
"address": "135.181.41.129/32",
"banned_until": 1649140716,
"ban_created": 1617604716
}
]
```
**PR: listbanned**
```
[
{
"address": "1.2.3.4/32",
"ban_created": 1617775773,
"banned_until": 1617862173,
"ban_duration": 86400,
"time_remaining": 86392
},
{
"address": "3.114.211.172/32",
"ban_created": 1617753165,
"banned_until": 1618357965,
"ban_duration": 604800,
"time_remaining": 582184
}
]
```
ACKs for top commit:
jonatack:
re-ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec
hebasto:
ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec, tested on Linux Mint 20.1 (x86_64).
MarcoFalke:
review ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec 🕙
Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
The order was broken in commit ffff4e7373f7c1260e6a8347b4ea1a99db4fff76
Since Qt 5.3.1 hash seeding is disabled for rcc.
See commit 5283a6c87beac5a43f612786fefd6e43f2c70bf6.
Fix make deploy for arm64-darwin
The intent here is to allow checking ComputeBlockVersion behaviour with
each deployment, rather than only testdummy on mainnet. This commit does
the trivial refactoring component of that change.
This generalises the ComputeBlockVersion test so that it can apply to
any activation parameters we might set, and checks all the parameters
set for each deployment on each chain, to simultaneously ensure that the
deployments we have configured work sensibly, and that the test code
does not suffer bitrot in the event that all interesting deployments
are buried.
Simplify the versionbits unit test slightly to make the next set of
changes a little easier to follow.
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.
Details: https://github.com/Homebrew/discussions/discussions/691
b7381552cd4f965c45f1560d9cfc2fb09dbfcc1d ci: Fix macOS brew install command (Hennadii Stepanov)
Pull request description:
A solution for https://bintray.com shutdown.
Details: https://github.com/Homebrew/discussions/discussions/691.
ACKs for top commit:
jamesob:
ACK https://github.com/bitcoin/bitcoin/pull/21663/commits/b7381552cd4f965c45f1560d9cfc2fb09dbfcc1d
Tree-SHA512: 4570c297421ae66043348acbe2f1e50b000e700406b38bc813c80714d4f109c6cadecff571ecbd8c2f0094ebdc959486fc55022ba6545bd592683a421e9c0655
Since we want tests to run quickly, and since tests do a lot more db
operations than expected we expect to see in actual usage, we disable
sqlite's syncing behavior to make db operations run much faster. This
syncing behavior is necessary for normal operation as it helps guarantee
that data won't become lost or corrupted, but in tests, we don't care
about that.
08151e19d97f0407e99826007df3ba5b257129b4 doc: Fix name of script guix-build (Stephan Oeste)
Pull request description:
ACKs for top commit:
hebasto:
ACK 08151e19d97f0407e99826007df3ba5b257129b4
jarolrod:
ACK 08151e19d97f0407e99826007df3ba5b257129b4
Tree-SHA512: 50e52f91b489db6616b5c9a993474bc1b8c196c3cac4fd5ded7c8fece5a7d72f85d9f566ee6a3df56a132a22a91dd72801ce849ec5e430a7850ff05abcab6b37
725d7ae0494d4a45f5a840bbbd19c008a7363965 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467330b2c6bb0d44786020c55f1aa75f9 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd619ac85ab35eff376c03136fe4a9f04 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507bdc716e5236b1d9880e648147e0af9 Add MissingDataBehavior and make TransactionSignatureChecker handle it (Pieter Wuille)
Pull request description:
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
* P2WPKH/P2WSH hashes need the spent amount
* P2TR hashes need all spent outputs (amount + scriptPubKey)
Missing amounts are treated as -1 (thus leading to unexpected signature failures), while missing outputs in P2TR validation cause assertion failure. This is hard to extend for signing support, and also quite ugly in general.
In this PR, an explicit configuration option to {Mutable,}TransactionSignatureChecker is added (MissingDataBehavior enum class) to either select ASSERT_FAIL or FAIL. Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL.
The existence of the ASSERT_FAIL option is really just an abundance of caution. Always using FAIL should be just fine, but if there were for some reason a code path in consensus code was introduced that misses certain data, I think we prefer as assertion failure over silently introducing a consensus change.
Potentially useful follow-ups (not for this PR, in my preference):
* Having an explicit script validation error code for missing data.
* Having a MissingDataBehavior::SUCCEED option as well, for use in script/sign.cpp DataFromTransaction (if a signature is present in a witness, and we don't have enough data to fully validate it, we should probably treat it as valid and not touch it).
ACKs for top commit:
sanket1729:
reACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Sjors:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
achow101:
re-ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
benthecarman:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
fjahr:
Code review ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
1c1467f51b6dda92dec974eb59026c2c5ba79ed6 i2p: cancel the Accept() method if waiting on the socket errors (Vasil Dimov)
Pull request description:
If `Sock::Wait()` fails, then cancel the `Accept()` method.
Not checking the return value may cause an uninitialized read a few lines below when we read the `occurred` variable.
[Spotted](https://github.com/bitcoin/bitcoin/pull/21630#issuecomment-814765659) by MarcoFalke, thanks!
ACKs for top commit:
laanwj:
Code review ACK 1c1467f51b6dda92dec974eb59026c2c5ba79ed6
practicalswift:
cr ACK 1c1467f51b6dda92dec974eb59026c2c5ba79ed6: patch looks correct and agree with laanwj that `[[nodiscard]]` can be taken in a follow-up PR :)
Tree-SHA512: 57fa8a03a4e055999e23121cd9ed1566a585ece0cf68b74223d8c902804cb6890218c9356d60e0560ccacc6c8542a526356c226ebd48e7b299b4572be312d49b
88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee rpc: add help for enumeratesigners and walletdisplayaddress (Sjors Provoost)
b0db187e5b30a491c9f95685430a82a1e35e921d ci: use --enable-external-signer instead of --with-boost-process (Sjors Provoost)
b54b2e7b1a171203404bd41853372c73f2c64532 Move external signer out of wallet module (Sjors Provoost)
Pull request description:
In addition, this PR enables external signer testing on CI.
This PR moves the ExternalSigner class and RPC methods out of the wallet module.
The `enumeratesigners` RPC can be used without a wallet since #21417. With additional modifications external signers could be used without a wallet in general, e.g. via `signrawtransaction`.
The `signerdisplayaddress` RPC is ranamed to `walletdisplayaddress` because it requires wallet context. A future `displayaddress` RPC call without wallet context could take a descriptor argument.
This commit fixes a `rpc_help.py` failure when configured with `--disable-wallet`.
ACKs for top commit:
ryanofsky:
Code review ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee
fanquake:
ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee
Tree-SHA512: 3242a24e22313aed97eee32a520bfcb1c17495ba32a2b8e06a5e151e2611320e2da5ef35b572d84623af0a49a210d2f9377a2531250868d1a0ccf3e144352a97
fa4f0b301ba5c07f1271fa7129d07683014d169b ci: Fix previous releases cache order (MarcoFalke)
Pull request description:
ACKs for top commit:
hebasto:
ACK fa4f0b301ba5c07f1271fa7129d07683014d169b
Tree-SHA512: 997e46e5432abb1f24c0762dab6366e173a8afd13a02d655691dbe8d6f532f4c5748e0874a7d158d7e171b2991ed2ce9cfc1982a5d9cd30d1dbb30f43452025d
Rather than 3 different messages that are confusing / leak
implementation details, use a single message, that is similar to other
wallet related messages. i.e:
"Compiled without sqlite support (required for descriptor wallets)".
This is undocumented and unused.
It's not clear why this need it's own exception class, as opposed to just
throwing std::runtime_error().
223b1ba7d90509a47ea07af46f4b9c3b8efbc9f8 doc: Use CONFIG_SITE instead of --prefix (Hennadii Stepanov)
Pull request description:
The current examples of `--prefix=...` option usage to point `configure` script to appropriate `depends` directory is not [standard](https://www.gnu.org/prep/standards/html_node/Directory-Variables.html). This causes some [confusion](https://github.com/bitcoin/bitcoin/pull/16691) and a bit of inconvenience.
Consider a CentOS 7 32 bit system. Packages `libdb4-devel`, `libdb4-cxx-devel`, `miniupnpc-devel` and `zeromq-devel` are unavailable from repos. After recommended build with depends:
```
cd depends
make
cd ..
./autogen.sh
./configure --prefix=$PWD/depends/i686-pc-linux-gnu
make
```
a user is unable to `make install` compiled binaries neither locally (to `~/.local`) nor system-wide (to `/usr/local`) as `--prefix` is set already.
Meanwhile, the standard approach with using [`config.site`](https://www.gnu.org/software/automake/manual/html_node/config_002esite.html) files allows both possibilities:
```
cd depends
make
cd ..
./autogen.sh
CONFIG_SITE=$PWD/depends/i686-pc-linux-gnu/share/config.site ./configure --prefix ~/.local
make
make install
```
or
```
CONFIG_SITE=$PWD/depends/i686-pc-linux-gnu/share/config.site ./configure
make
sudo make install # install to /usr/local
```
Moreover, this approach is used in [Gitian descriptors](https://github.com/bitcoin/bitcoin/tree/master/contrib/gitian-descriptors) already.
ACKs for top commit:
practicalswift:
ACK 223b1ba7d90509a47ea07af46f4b9c3b8efbc9f8: patch looks correct
fanquake:
ACK 223b1ba7d90509a47ea07af46f4b9c3b8efbc9f8
Tree-SHA512: 46d97924f0fc7e95ee4566737cf7c2ae805ca500e5c49af9aa99ecc3acede4b00329bc727a110aa1b62618dfbf5d1ca2234e736f16fbdf96d6ece5f821712f54
003929c0d55532038d5bf6fc0ff4a20628710fae refactor: add [[noreturn]] attribute where applicable (fanquake)
Pull request description:
Similar to #10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.
ACKs for top commit:
vasild:
ACK 003929c0d55532038d5bf6fc0ff4a20628710fae
Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d51bb0e25eb3fbd716881fa55527dc7 blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad24d3c9515d3f9c136af4071cbd79055 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3447a3645e7958c7dc4e1946a69cb9c move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a14744e7d7f7746e861aabd9cf938f61 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f99f91603e08ea858f972516f1d669a refactor: Move load block thread into ChainstateManager (MarcoFalke)
Pull request description:
This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
* Breaking down the massive files init.cpp and validation.cpp into logical units
* Creating a standalone-module to reduce the mental complexity
* Pave the way to fix validation related circular dependencies
* Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
ACKs for top commit:
promag:
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
jamesob:
ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
ryanofsky:
Code review ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
41f891da508114f1fd4df30b4068073ec30abc2a tests: Skip SQLite fsyncs while testing (Andrew Chow)
Pull request description:
Since we want tests to run quickly, and since tests do a lot more db operations than expected we expect to see in actual usage, we disable sqlite's syncing behavior to make db operations run much faster. This syncing behavior is necessary for normal operation as it helps guarantee that data won't become lost or corrupted, but in tests, we don't care about that.
Fixes #21628
ACKs for top commit:
vasild:
ACK 41f891da508114f1fd4df30b4068073ec30abc2a
Tree-SHA512: f36f969a182c622691ae5113573a3250e8d367437e83a1a9d3d2b55dd3a9cdf3c6474169a7bd271007bb9ce47f585aa7a6aeae6eebbaeb02d79409b02f47fd8b
Passing ADDITIONAL_GUIX_COMMON_FLAGS="--no-substitutes --bootstrap" as
suggested doesn't work:
```bash
...outputting in: '/bitcoin/guix-build-a1f0b8b62eb8/output/x86_64-linux-gnu'
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
guix time-machine: error: bootstrap: unrecognized option
```
and I think bootstrapping is more than covered in the preceding "Choose
your security model" section.
c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f external_signer: remove ExternalSignerException (fanquake)
9e0b199b976617edeb1c58d4203df5f83a26c1e3 external_signer: use const where appropriate (fanquake)
aaa4e5a45bd9ec5563ffa7b9e0d46d2de3cb9242 wallet: remove CWallet::GetExternalSigner() (fanquake)
06a0673351282fff1673f3679a7cad9a7faaf987 external_signer: remove ignore_errors from Enumerate() (fanquake)
8fdbb899b84a2be85e632e45f08b222db02395d9 refactor: unify external wallet runtime errors (fanquake)
f4652bf1259d5c52ff0d500c732f40ba41256817 refactor: add missing includes to external signer code (fanquake)
54569cc6d6f54788169061004026e62e1c08440e refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs (fanquake)
Pull request description:
These are a few followups after #21467.
ACKs for top commit:
Sjors:
tACK c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/21666/commits/c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f
Tree-SHA512: 3d5ac5df81680075e71e0e4a7595c520d746c3e37f016cf168c1e10da15541ebb1595aecaf2c08575636e9ff77d499644cae53180232b7049cfae0b923106e4e
I can no longer observe the need for this suppression.
This reverts commit fa1fc536bb26471fd2a6fe8d12f98cf53c646306.
fadea0bf371a38620b7f1f93f87d1da76d3314e0 Revert "test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles" (MarcoFalke)
fadbd9988590ba94e3fd2d87d773f3b09d73ef46 test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke)
Pull request description:
The double lock warnings appeared in #19041, but they didn't make any sense. Also, our sync module would detect double locks, if there were any.
Bumping to clang-12 allows us to remove the spurious suppressions needed to run the tests, so do that.
ACKs for top commit:
practicalswift:
cr ACK fadea0bf371a38620b7f1f93f87d1da76d3314e0 assuming CI passes and more specifically that newer Clang agrees that these TSan suppressions are no longer needed.
Tree-SHA512: c411221a4b74d0af6ca8d686639b4f40b41c15906ccbb6647e8d569d6ab088264faafe075e1ac9523d5c0024b85f15a597bb3eedc7f07d4f5816245f75cfc08b
This change fixes locally running tests.
b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot (Hennadii Stepanov)
1ac2bc7ac070dfd1df1872d759540b0c92495301 qt: Handle exceptions in TransactionView::bumpFee slot (Hennadii Stepanov)
bc00e13bc800863641b3e1e64732a38418d3022f qt: Handle exceptions in WalletModel::pollBalanceChanged slot (Hennadii Stepanov)
eb6156ba1b4c303eb597e3fc4a9e42ce45e6e78d qt: Handle exceptions in BitcoinGUI::addWallet slot (Hennadii Stepanov)
f7e260a471010e2d656fbc5ea8c310f6d94c26b9 qt: Add GUIUtil::ExceptionSafeConnect function (Hennadii Stepanov)
64a8755af396f1c2791018510e22b58114e68594 qt: Add BitcoinApplication::handleNonFatalException function (Hennadii Stepanov)
af7e365b1516d660d271475fdfe0c20ae09e66a8 qt: Make PACKAGE_BUGREPORT link clickable (Hennadii Stepanov)
Pull request description:
This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/18897, and is based on Russ' [idea](https://github.com/bitcoin/bitcoin/pull/18897#pullrequestreview-418703664):
> IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ...
Related issues
- #91
- https://github.com/bitcoin/bitcoin/issues/18643
Qt docs: https://doc.qt.io/qt-5.12/exceptionsafety.html#exceptions-in-client-code
With this PR the GUI handles the wallet-related exception, and:
- display it to a user:
- prints a message to `stderr`:
```
************************
EXCEPTION: 18NonFatalCheckError
wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
bitcoin in QPushButton->SendCoinsDialog
```
- writes a message to the `debug.log`
- and, if the exception is a non-fatal error, leaves the main window running.
ACKs for top commit:
laanwj:
Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd
ryanofsky:
Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd. This is great! I think more improvements are possible but implementation is very clean and I love how targeted each commit is. Changes since last review: adding more explanatory text, making links clickable, reorganizing.
Tree-SHA512: a9f2a2ee8e64b993b0dbc454edcbc39c68c8852abb5dc1feb58f601c0e0e8014dca81c72733aa3fb07b619c6f49b823ed20c7d79cc92088a3abe040ed2149727
fa73ce6e653d00824eb68f772fd29b7f8fb93d84 Fix assumeutxo crash due to truncated file (MarcoFalke)
Pull request description:
ACKs for top commit:
jamesob:
ACK fa73ce6e653d00824eb68f772fd29b7f8fb93d84
ryanofsky:
Code review ACK fa73ce6e653d00824eb68f772fd29b7f8fb93d84. Easy fix. It seems like this could have been caught in review, though.
Tree-SHA512: 3a98687c386e3995114ddf0ad7194fadd9520989290681ef703b578e3ca21aee51eadfb83aa38a489bac13d12709ea137b9b184b08e5bfa2919cca177aab90be
11115c8ee5d628f301c3f9a1ffe3afe37ac831fd ci: Only cache depends/sdk-sources for macos/apk task in cirrus (MarcoFalke)
Pull request description:
Only macos needs the sdk-sources, so move it there (and remove it from showing up in the other tasks)
ACKs for top commit:
hebasto:
ACK 11115c8ee5d628f301c3f9a1ffe3afe37ac831fd
Tree-SHA512: 9e80e74016052e7b00314e191acc84c5e400bbf2e0d93ee7b1494b6be99e671cc326ddaa1fbded82f0912a9e338ad1891019dce78199b08c5180af42cb59729a
f2ef5a8afd5da2fb7775dafdee15e6ac532d8fe4 test: Fix TSan suppression (Hennadii Stepanov)
Pull request description:
This PR is a #21669 follow up, and fixes [locally running `make check`](https://github.com/bitcoin/bitcoin/pull/21669#issuecomment-819403540).
ACKs for top commit:
MarcoFalke:
cr ACK f2ef5a8afd5da2fb7775dafdee15e6ac532d8fe4
Tree-SHA512: bb0c4d1707c6194358d2e9abfed5aa8dd487e014199025fb89f6e5a66d774af041b46a03358a9a5412e1683675c05c42a3b719217d940412ee3fe1ed18a5274c
- The original Ensure*(const std::any& context) functions are kept and
the parameter renamed to ctx so that the scripted-diff in the
subsequent commit will work as expected
- The renaming avoids overloading mistakes arising out of the untyped
std::any argument.
-BEGIN VERIFY SCRIPT-
sed -i -E 's@Ensure([^(]+)(\((request\.|)context\))@EnsureAny\1\2@g' \
-- src/rest.cpp src/rpc/*.cpp
-END VERIFY SCRIPT-
The scripted-diff in the previous commit should have removed all calls
to functions like: Ensure(?!Any)\(const std::any& (context|ctx)\), so we
can remove them now.
Organize local variables/references such that:
1. There is always a `ChainstateManager` reference before any `LOCK(cs_main)`.
2. NodeContext references are used with Ensure*() functions introduced in
previous commit where appropriate to avoid duplicate assertions.
In all rest/rpc-related modules, if there are multiple calls to
ActiveChain{,State}(), and the calls fall under the same ::cs_main lock,
we can simply take a local reference and use/reuse it instead of calling
ActiveChain{,State}() again and again.
Doing it there will reduce code bloat and also ensure no test can "forget" to reset it
Such shortcuts are useless as pressing the Alt key closes a context menu
widget immediately.
This overloaded function was introduced in Qt 5.6 and makes code more
concise.
09b3e468480538977d2844ba083c66197c9d1d08 doc: remove boostrap info from GUIX_COMMON_FLAGS doc (fanquake)
Pull request description:
Passing `ADDITIONAL_GUIX_COMMON_FLAGS="--no-substitutes --bootstrap"` as suggested doesn't work:
```bash
...outputting in: '/bitcoin/guix-build-a1f0b8b62eb8/output/x86_64-linux-gnu'
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
guix time-machine: error: bootstrap: unrecognized option
```
and I think bootstrapping is more than covered in the preceding "Choose your security model" section.
ACKs for top commit:
dongcarl:
ACK 09b3e468480538977d2844ba083c66197c9d1d08
jarolrod:
ACK 09b3e468480538977d2844ba083c66197c9d1d08
laanwj:
Documentation review ACK 09b3e468480538977d2844ba083c66197c9d1d08
Tree-SHA512: e533a0b925a2ec091884ec04313f56376b4e85f615c8853dd51840181493d61bd01fee1c019e34880c32d7896e4871ea77e63398ba02b022f614c03bb16531aa
ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK https://github.com/bitcoin/bitcoin/pull/21377/commits/ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jnewbery:
utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
MarcoFalke:
review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
achow101:
re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
gmaxwell:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
benthecarman:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
Sjors:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jonatack:
Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
6262182b3f1c9540291fb8de3bf7a785e7113c55 Avoid use of low file descriptor ids (which may be in use) in FuzzedSock and StaticContentsSock (practicalswift)
Pull request description:
Avoid use of low file descriptor ids (which may be in use) in `FuzzedSock`.
Context: https://github.com/bitcoin/bitcoin/pull/21630/files#r610694541
ACKs for top commit:
vasild:
ACK 6262182b3f1c9540291fb8de3bf7a785e7113c55
Tree-SHA512: e622acb4d01446c3db01adbbbb779038be7247e13f3f4e72c614bc2880c3efd710fd3b189f87abb00f236fa5ddf91f4c215f420ca4eb08a97aaba31593254c3d
Move the `FuzzedSock`'s implementation from `src/test/fuzz/util.h` to
`src/test/fuzz/util.cpp`.
A separate interface and implementation make the code more readable for
consumers who don't need to (better not) know the implementation
details.
The former is shorter and ends up with a "random" bool anyway.
fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1 test: Reset mocktime in the common setup (MarcoFalke)
fa78590a8fffdfc7e98ddb1f81218f05b1935a0a test: Use mocktime to avoid intermittent failure (MarcoFalke)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/21602#discussion_r611176103
ACKs for top commit:
jonatack:
Code review ACK fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1
jarolrod:
ACK fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1
Tree-SHA512: 4967e006f3d2c4eb92f03c9086a6abe3190ad54755d251c30d20422c574bb1a154c06f3d5bcb0d4deaa3c4abfd3864d743b71d84897edd358e829bb42233ad12
f979b3237f1cfc28f9c4ccb07beab558d5357a55 Add mainnet and testnet taproot activation params (Andrew Chow)
Pull request description:
Adds the activation parameters for taproot as specified in https://github.com/bitcoin/bips/pull/1104
ACKs for top commit:
gmaxwell:
utACK f979b3237f1cfc28f9c4ccb07beab558d5357a55
ajtowns:
ACK f979b3237f1cfc28f9c4ccb07beab558d5357a55
instagibbs:
ACK f979b32
clarkmoody:
ACK f979b32
Sjors:
ACK f979b3237f1cfc28f9c4ccb07beab558d5357a55
jonatack:
utACK f979b3237f1cfc28f9c4ccb07beab558d5357a55 verified with the BIP draft
Tree-SHA512: f95538bcec46c36f9532a99fcf697b143083c25b2427dd578b88514add0a807371530c18f0a8ed040dc885ad6eca8234235e1d762f6f837eafc5daed856a9dcf
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
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
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
This allows to remove check that windows for the same bit are disjoint
This addresses https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611492633
* https://github.com/bitcoin/bitcoin/pull/21377#discussion_r609585080
* https://github.com/bitcoin/bitcoin/pull/21377#discussion_r613702341
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
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
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
-BEGIN VERIFY SCRIPT-
git ls-files src/test/getarg_tests.cpp | xargs sed -i "s/m_args/m_local_args/g";
-END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
git ls-files src/test/dbwrapper_tests.cpp src/test/denialofservice_tests.cpp src/test/flatfile_tests.cpp src/test/fs_tests.cpp src/test/settings_tests.cpp src/test/util_tests.cpp | xargs sed -i 's/GetDataDir()/m_args.GetDataDirPath()/g';
-END VERIFY SCRIPT-
These changes in the test documentation reflect the changes introduced in #17921
a41149426168b8ea96099f10576022c6a09033d1 rpc: Improve getblock error message for invalid data type. (klementtan)
Pull request description:
Improve error messages for getblock invalid datatype.
fixes: #21717
ACKs for top commit:
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/21718/commits/a41149426168b8ea96099f10576022c6a09033d1
theStack:
ACK a41149426168b8ea96099f10576022c6a09033d1
promag:
Code review ACK a41149426168b8ea96099f10576022c6a09033d1.
Tree-SHA512: 6e7d8290681e8ab375629f81669d0f8e0c21f9eb7ed9e2455cd19ea013e69b2d95fa7a9ee795315b2d5c60c96035c6cefc3d6e1039a06fd88c1dc7fe275ee6a1
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
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
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
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
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 https://github.com/bitcoin/bitcoin/commit/19db590d044efe7d474a16720e5b56e7b55db54c), 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
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
Division of MuHash objects are very expensive and multiplication relatively cheap. The whole idea of introducing and tracking numerator and denominators seperately as a representation of the internal state was so that divisions would be rare. So using divison in the Remove method did not make any sense and was just a silly mistake which is corrected here.
The index holds the values previously calculated in coinstats.cpp
for each block, representing the state of the UTXO set at each
height.
This additional data will automatically be returned if the coinstats index is used.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
`vcpkg integrate install` must be executed so that msbuild will automatically install external dependencies.
It was removed in https://github.com/bitcoin/bitcoin/commit/712f95d3324d02310dd468e7bfd1e1b0df432e77
It was originally added in https://github.com/bitcoin/bitcoin/commit/76445677586a4c2fa72606b662269a4390c2e71f
f2f2541ee7ad36191515ff351b667fe12a2ab871 remove executable flag for src/net_processing.cpp (Sebastian Falbesoner)
Pull request description:
The file permissions for `src/net_processing.cpp` have been changed in #21713, as discovered by fanquake (https://github.com/bitcoin/bitcoin/pull/21713#issuecomment-822245960). This PR removes the executable flag again.
ACKs for top commit:
kiminuo:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871 :)
jnewbery:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871
promag:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871.
Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb
bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5 Change ClearDataDirPathCache() to ArgsManager.ClearPathCache(). (Kiminuo)
b4190eff72c00e384ad238f9c2f10c8b91be969b Change GetBlocksDir() to ArgsManager.GetBlocksDirPath(). (Kiminuo)
83292e2a700afbf39936bd67bb89fab5398d0066 scripted-diff: Modify unit tests to use the ArgsManager in the BasicTestingSetup class instead of implicitly relying on gArgs. (Kiminuo)
55c68e6f011ee604c8a65b9bca668eb4dec452aa scripted-diff: Replace m_args with m_local_args in getarg_tests.cpp (Kiminuo)
511ce3a26b3b78e14acd0d85496b5422a236cf63 BasicTestingSetup: Add ArgsManager. (Kiminuo)
1cb52ba0656e78ca6c2ef84b1558198ad113b76a Modify "util_datadir" unit test to not use gArgs. (Kiminuo)
1add318704108faa98f5b1b8e9c96d960e9d23a8 Move GetDataDir(fNetSpecific) implementation to ArgsManager. (Kiminuo)
70cdf679f8e665dbdc3301873a0267fe9faa72cd Move StripRedundantLastElementsOfPath before ArgsManager class. (Kiminuo)
Pull request description:
This PR attempts to contribute to "Remove gArgs" (#21005).
Main changes:
* `GetDataDir()` function is moved to `ArgsManager.GetDataDirPath()`.
* `GetBlocksDir()` function is moved to `ArgsManager.GetBlocksDirPath()`.
ACKs for top commit:
ryanofsky:
Code review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5. Just minor const/naming changes and splitting/scripting commits since last review
MarcoFalke:
review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5 📓
hebasto:
re-ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5, addressed comments, and two commits made scripted-diffs since my [previous](https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-638270583) review.
Tree-SHA512: ba9408c22129d6572beaa103dca0324131766f06d562bb7d6b9e214a0a4d40b0216ce861384562bde24b744003b3fbe6fac239061c8fd798abd3981ebc1b9019
fa8eaee6a8531db970cc84436bf2ae8150a58642 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1f12612831ad5b0f0a767d87bd2d024 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871c3c9fde462e64e3ef3be937e585084 test: Check that no versionbits are re-used (MarcoFalke)
Pull request description:
ACKs for top commit:
jnewbery:
Code review ACK fa8eaee6a8531db970cc84436bf2ae8150a58642
ajtowns:
ACK fa8eaee6a8531db970cc84436bf2ae8150a58642 code review only
Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
0b188b751f970027c52729e0c223cc9257669322 Clean up context dependent checks in descriptor parsing (Pieter Wuille)
33275a96490445e293c322a29a3b146ccb91083c refactor: move uncompressed-permitted logic into ParsePubkey* (Pieter Wuille)
17e006ff8d5e42f22474c5191d1b745bbc97571f refactor: split off subscript logic from ToStringHelper (Pieter Wuille)
6ba5dda0c9de75196c6a427d9e59d39e5a41bff7 Account for key cache indices in subexpressions (Pieter Wuille)
4441c6f3c046c0b28ce3f0ca6d938af71d797586 Make DescriptorImpl support multiple subscripts (Pieter Wuille)
a917478db0788b244c0c799b98bf67a94df7035e refactor: move population of out.scripts from ExpandHelper to MakeScripts (Pieter Wuille)
84f3939ece9f4901141b28fd2dd6e3899d01d66e Remove support for subdescriptors expanding to multiple scripts (Pieter Wuille)
Pull request description:
These are a few refactors and non-invasive improvements to the descriptors code to prepare for adding Taproot descriptors.
None of the commits change behavior in any way, except the last one which improves error reporting a bit.
ACKs for top commit:
S3RK:
reACK 0b188b7
Sjors:
re-ACK 0b188b7
achow101:
re-ACK 0b188b751f970027c52729e0c223cc9257669322
Tree-SHA512: cb4e999134aa2bace0e13d4883454c65bcf1369e1c8585d93cc6444ddc245f3def5a628d58af7dab577e9d5a4a75d3bb46f766421fcc8cc5c85c01a11f148b3f
It looks like #21238 introduced a silent merge conflict in the
documentation, which fails with `-Wdocumentation` in the CI.
Credit to João Barbosa (promag) for the suggestions.
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
e5faec65bd06a3b14175aca3040290f343bd6e9c doc: Fix doxygen comment silent merge conflict in descriptor.cpp (W. J. van der Laan)
Pull request description:
It looks like #21238 introduced a silent merge conflict in the documentation, which fails with `-Wdocumentation` in the CI.
(please merge only if CI passes)
ACKs for top commit:
ajtowns:
ACK e5faec65bd06a3b14175aca3040290f343bd6e9c -- fixed it for me
meshcollider:
ACK e5faec65bd06a3b14175aca3040290f343bd6e9c modulo CI
Tree-SHA512: b07d50fd12aa7c239a92aad8ef29f4e88583c3ce701ebedba7c426aac4981c79113adc4670b7d055ab9535a28bdc3f9a30e6ca1b1ed0d7b9a333a3d9c4b40d8a
Changes from Ubuntu Focal to Hirsute to get clang-12.
Generally, it can be expected that more recent compilers have
better support for sanitizers.
Otherwise it is not possible to run bench_bitcoin with clang-12 + ASAN
compiled.
Output:
$ src/bench/bench_bitcoin
bench/nanobench.h:1107:15: runtime error: left shift of 4982565676696827473 by 27 places cannot be represented in type 'uint64_t' (aka 'unsigned long')
#0 0x5623d6a13137 in ankerl::nanobench::Rng::rotl(unsigned long, unsigned int) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:1107:15
#1 0x5623d6a13137 in ankerl::nanobench::Rng::operator()() /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:1075:10
#2 0x5623d6a05c5b in ankerl::nanobench::Rng::Rng(unsigned long) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:3135:9
#3 0x5623d6a0ca51 in ankerl::nanobench::detail::IterationLogic::Impl::Impl(ankerl::nanobench::Bench const&) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:2206:13
#4 0x5623d69f8f73 in ankerl::nanobench::detail::IterationLogic::IterationLogic(ankerl::nanobench::Bench const&) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:2215:18
#5 0x5623d690f165 in ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<AddrManAdd(ankerl::nanobench::Bench&)::$_0>(AddrManAdd(ankerl::nanobench::Bench&)::$_0&&) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./bench/nanobench.h:1114:28
#6 0x5623d690e26e in AddrManAdd(ankerl::nanobench::Bench&) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bench/addrman.cpp:76:11
#7 0x5623d69279d6 in void std::__invoke_impl<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>(std::__invoke_other, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:60:14
#8 0x5623d6927921 in std::enable_if<is_invocable_r_v<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>, void>::type std::__invoke_r<void, void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&>(void (*&)(ankerl::nanobench::Bench&), ankerl::nanobench::Bench&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:110:2
#9 0x5623d692775f in std::_Function_handler<void (ankerl::nanobench::Bench&), void (*)(ankerl::nanobench::Bench&)>::_M_invoke(std::_Any_data const&, ankerl::nanobench::Bench&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:291:9
#10 0x5623d692dbd5 in std::function<void (ankerl::nanobench::Bench&)>::operator()(ankerl::nanobench::Bench&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:622:14
#11 0x5623d692cd44 in benchmark::BenchRunner::RunAll(benchmark::Args const&) /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bench/bench.cpp:65:13
#12 0x5623d69282bf in main /bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bench/bench_bitcoin.cpp:63:5
#13 0x7f6812010564 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28564)
#14 0x5623d685f4dd in _start (/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bench/bench_bitcoin+0x13754dd)
SUMMARY: UndefinedBehaviorSanitizer: invalid-shift-base bench/nanobench.h:1107:15 in
$ clang --version
Ubuntu clang version 12.0.0-1ubuntu1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
de17d245b7462784f9d36591b0f346aa6b2e9b8c Re-add command to install vcpkg (dplusplus1024)
Pull request description:
`vcpkg integrate install` must be executed so that msbuild will automatically install external dependencies.
It was removed in https://github.com/bitcoin/bitcoin/commit/712f95d3324d02310dd468e7bfd1e1b0df432e77
It was originally added in https://github.com/bitcoin/bitcoin/commit/76445677586a4c2fa72606b662269a4390c2e71f
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
sipsorcery:
ACK de17d245b7462784f9d36591b0f346aa6b2e9b8c.
hebasto:
ACK de17d245b7462784f9d36591b0f346aa6b2e9b8c, I use the same in #21551.
Tree-SHA512: b32428e450cf76297a0e78b5ffb0933ddcfcce010dbd6f5b87d434bf9c437258edb5b817161f6255d5d60523f79f8513a1fe3e92ba62e3fd3786ab042aaae849
06c43201a714b0426cc68b2fd5c681e5df10af99 cli: use C++17 std::array class template argument deduction (CTAD) (Jon Atack)
edf3167151f7a6d08cf733b4e230e2d745819ac8 addrinfo: raise helpfully on server error or incompatible server version (Jon Atack)
bb85cbc4f7638a85049658ed951a0e06e7959cd4 doc: add cli -addrinfo release note (Jon Atack)
5056a37624b64588b277419f7ed8c325477a8ec7 cli: add -addrinfo command (Jon Atack)
db4d2c282afd46709792aaf2d36ffbfc1745b776 cli: create AddrinfoRequestHandler class (Jon Atack)
Pull request description:
While looking at issue #21351, it turned out that the problem was a lack of tor v3 addresses known to the node. It became clear (e.g. https://github.com/bitcoin/bitcoin/issues/21351#issuecomment-811004779) that a CLI command returning the number of addresses the node knows per network (with a tor v2 / v3 breakdown) would be very helpful. This patch adds that.
`-addrinfo` is useful to see if your node knows enough addresses in a network to use options like `-onlynet=<network>`, or to upgrade to the upcoming tor release that no longer supports tor v2, for which you'll need to be sure your node knows enough tor v3 peers.
```
$ bitcoin-cli --help | grep -A1 addrinfo
-addrinfo
Get the number of addresses known to the node, per network and total.
$ bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 14406,
"ipv6": 2511,
"torv2": 5563,
"torv3": 2842,
"i2p": 8,
"total": 25330
}
}
$ bitcoin-cli -addrinfo 1
error: -addrinfo takes no arguments
```
This can be manually tested, for example, with commands like this:
```
$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length <= 22)) | .address' | wc -l
5563
$ bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".onion")) | select(.address | length > 22)) | .address' | wc -l
2842
$ bitcoin-cli getnodeaddresses 0 | jq '.[] | .address' | wc -l
25330
```
ACKs for top commit:
laanwj:
Tested ACK 06c43201a714b0426cc68b2fd5c681e5df10af99
Tree-SHA512: b668b47718a4ce052aff218789f3da629bca730592c18fcce9a51034d95a0a65f8e6da33dd47443cdd8f60c056c02696db175b0fe09a688e4385a76c1d8b7aeb
Details: https://docs.transifex.com/formats/xliff
The goal is to see the way the Transifex presents the added items to
translators using an intermediate XLIFF translation file.
As the bitcoin_en.xlf file is created by the `make -C src translate`
command, other translation-related files are also updated.
35d52397e72f3ab96a7797148666b501d50b445d Add bitcoin_en.xlf intermediate translation file to the repo (Hennadii Stepanov)
99686b65195fe990144be1a6326061e9c6ae506c qt [experimental]: Add a translation comment and a disambiguation string (Hennadii Stepanov)
f959b75e8c5964dec59a1dce937ee2244cce33df build: Add Qt lconvert tool to depends (Hennadii Stepanov)
2045e4cdd26920665300f21be5b08fe66bde4a41 build: Use XLIFF file to provide more context to Transifex translators (Hennadii Stepanov)
Pull request description:
Currently, only a class name is provided to the Transifex translators as a context. Neither `disambiguation` parameter of the `tr()` function nor [translator comments](https://doc.qt.io/qt-5/i18n-source-translation.html#translator-comments), being included as XML elements to `*.ts` translation files, are not parsed by the Transifex due to its [limited support](https://docs.transifex.com/formats/qt-ts) of such files.
This PR makes possible to provide all of the context details via an intermediate [XLIFF](https://docs.transifex.com/formats/xliff) translation file.
With this PR `make -C src translate` produces the `src/qt/locale/bitcoin_en.xlf` file which must be provided to the Transifex as a translation source instead of `src/qt/locale/bitcoin_en.ts`.
Closes #21465.
An example translatable string with additional `<context>` and `<note>` XML elements: https://github.com/bitcoin/bitcoin/blob/35d52397e72f3ab96a7797148666b501d50b445d/src/qt/locale/bitcoin_en.xlf#L126-L132
ACKs for top commit:
laanwj:
ACK 35d52397e72f3ab96a7797148666b501d50b445d
Tree-SHA512: cc19e3c09501d240153550d75d7697b5f824cb553f4223beaff66be4d3e6f98d7b5bb14f2d1e1d5ad014eaaa498a7f672e7ff0054ced53ace8c1e6f7e49f6d8a
f02ca7a354b69e3959d513cd36637507cbc33e78 Update msvc build to use Qt5.12.10 binaries. (Aaron Clauson)
Pull request description:
ACKs for top commit:
hebasto:
ACK f02ca7a354b69e3959d513cd36637507cbc33e78, I made a customized AppVeyor build with an artifact: https://ci.appveyor.com/project/hebasto/bitcoin/builds/38786868/artifacts
Tree-SHA512: 0f8998a5b72bca9a08fc5ec5c40b0b79e9247486f58f74824ebf045175d9e3ce7485c16d8de574b7316e79a8433af0646870abf4b0f3b47fc35a92f63a7b5dc9
16c157de3c316517e095994fa8d526253225a672 qt, refactor: Use better QMenu::addAction overloaded function (Hennadii Stepanov)
79311750b58d650d49a3f0edd59d31dd132ab8c0 qt: Do not assign Alt+<KEY> shortcuts to context menu actions (Hennadii Stepanov)
963e12058f3ca3cdaeefd9aa5a8305fa41afd1a0 qt: Drop menu separator that separates nothing (Hennadii Stepanov)
1398a6536c710368d9f1d0cf6e280fe63d07c9f0 qt, refactor: Make AddressBookPage::deleteAction a local variable (Hennadii Stepanov)
Pull request description:
This PR:
1. removes useless `Alt` + `<KEY>` shortcuts from context menu items
2. replaces 3 lines of code with the only call of [`QMenu::addAction`](https://doc.qt.io/qt-5/qmenu.html#addAction-5) for each context menu item (it became possible since https://github.com/bitcoin/bitcoin/pull/21286 was merged)
3. makes other minor cleanups
No behavior change.
ACKs for top commit:
kristapsk:
ACK 16c157de3c316517e095994fa8d526253225a672
promag:
Code review ACK 16c157de3c316517e095994fa8d526253225a672. Nice code cleanup that takes advantage of more recent Qt API.
jarolrod:
ACK 16c157de3c316517e095994fa8d526253225a672
Tree-SHA512: e5555fe957058cc67b351aaf9f09fe3635edb2d07a2223d3093913a25607ae538f0a2fde84c0b0cd43e7475b248949548eb4a5d4b21d8f7391fa2fa8541c04ff
fafb68add5e16e8bd5b9428bcffcaee2639747cf refactor: Add and use EnsureConnman in rpc code (MarcoFalke)
faabeb854a6e46b46e4f26b22dc2c81e68e2d863 refactor: Mark member functions const (MarcoFalke)
Pull request description:
This removes the 10 occurrences of `throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");` and replaces them with `EnsureConnman`.
ACKs for top commit:
jarolrod:
re-ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
theStack:
ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
ryanofsky:
Code review ACK fafb68add5e16e8bd5b9428bcffcaee2639747cf
Tree-SHA512: 84c63cfe31e548645d906f7191a3526c7bea99ed0d54c2a75c2041452a44fe149ede343d8e1943b0e7770816c828bb047dfec8bc541a1f2b89920a126ee54d68
7abac98d3e3c1bc8ad66cb5c05184b9c5cc674d5 configure: Support -f{debug,macro}-prefix-map (Anthony Towns)
Pull request description:
When bitcoin is checked out in two directories (eg via git worktree) object files between the two will differ due to the full path being included in the debug section. `-fdebug-prefix-map` is used to replace this with "." to avoid this unnecessary difference and allow ccache to share objects between worktrees (provided the source and compile options are the same).
Also provide `-fmacro-prefix-map` if supported so that the working dir is not encoded in `__FILE__` macros.
ACKs for top commit:
practicalswift:
cr ACK 7abac98d3e3c1bc8ad66cb5c05184b9c5cc674d5: patch looks correct
fanquake:
ACK 7abac98d3e3c1bc8ad66cb5c05184b9c5cc674d5
Tree-SHA512: b6a37c1728ec3b2e552f244da0e66db113c1e7662c7ac502e12ff466f3dbfbfefae12695ca135137c50dbb1c4c5d84059116c0cd09b391a17466dc77b8726679
c799a19b4bb8d0cc5ffd9b49746b5b267745c9b5 build, qt: No longer need to set QT_RCC_TEST=1 for determinism (Hennadii Stepanov)
Pull request description:
The Qt Resource Compiler (rcc) output order relies on [`QHash`](https://doc.qt.io/qt-5/qhash.html):
> This randomization of `QHash` is enabled by default. Even though programs should never depend on a particular `QHash` ordering, there may be situations where you temporarily need deterministic behavior, for example for debugging or regression testing. To disable the randomization, define the environment variable `QT_HASH_SEED` to have the value 0.
Since #3620 we use `QT_RCC_TEST=1` to achieve a deterministic output.
Since Qt 5.3.1 hash seeding is disabled for all of the bootstrapped tools, including rcc. Therefore, `QT_RCC_TEST=1` is no longer needed.
See commit [5283a6c87beac5a43f612786fefd6e43f2c70bf6](https://github.com/qt/qtbase/commit/5283a6c87beac5a43f612786fefd6e43f2c70bf6).
ACKs for top commit:
fanquake:
ACK c799a19b4bb8d0cc5ffd9b49746b5b267745c9b5
Tree-SHA512: 9d116ac1e8c605ee3e8ed7f618586f0de85d8b06bbbb70fe8c298939ce203d2a7e97264a9afac037179993ab54c5f69a65ebb9ab27ca7f45acb963011bd45743
Opening the 22.x translations early because of experimentation with the
new xliff translations format.
In this context, change file_filter to `xlf` as well as the files
pulled with `tx pull` are that format now (the setting only affects the naming
not the format of the files).
Tree-SHA512: e0c18aa5e6cbd4428d24324fee8e5761b70dae51d0236277577aded719798c6a32fc81c0598f280321f2816629e33a334f61f9e7f6180c4074abfda6550cefbe
Moves setting up the addr message into a repeatable function, and breaks up the
existing tests into separate functions for legibility.
Also reduces mocktime to prevent idle disconnects
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
417305991a0573484f4aa3820103d8b991cb8f81 qt: Bump transifex slug for 22.x (W. J. van der Laan)
Pull request description:
Opening the 22.x translations early because of experimentation with the new xliff translations format. So change the slug so that the `tx` tool will fetch the right translation.
In this context, change `file_filter` to use `xlf` as well as the files pulled with `tx pull` are that format now. The setting only affects the naming not the format of the files.
ACKs for top commit:
hebasto:
ACK 417305991a0573484f4aa3820103d8b991cb8f81, I have reviewed the code and it looks OK, I agree it can be merged. All changes are in accordance with the Transifex [docs](https://docs.transifex.com/client/client-configuration).
Tree-SHA512: e0c18aa5e6cbd4428d24324fee8e5761b70dae51d0236277577aded719798c6a32fc81c0598f280321f2816629e33a334f61f9e7f6180c4074abfda6550cefbe
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
Instead of rewinding blocks, we request that the user restarts with
-reindex
58580a827d10c0675c3483e2aeca1d3ab8050ae0 net: Avoid calling getnameinfo when formatting IPv4 addresses in CNetAddr::ToStringIP (practicalswift)
58580573843858068db69a378b6e9363889b1f6d net: Add IPv4ToString (we already have IPv6ToString) (practicalswift)
Pull request description:
Avoid calling `getnameinfo` when formatting IPv4 addresses in `CNetAddr::ToStringIP`.
ACKs for top commit:
naumenkogs:
ACK 58580a827d10c0675c3483e2aeca1d3ab8050ae0
0xB10C:
ACK 58580a827d10c0675c3483e2aeca1d3ab8050ae0
vasild:
ACK 58580a827d10c0675c3483e2aeca1d3ab8050ae0
Tree-SHA512: 25e3c416acb74908d001baf1cf64c04cbc0d94ce8e7ce5a601f1343062d5d748cb406a3404e6f2b6e7e979c6300b38439e1bfd70ea90ec8c0ec2d7568f09fbcd
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
https://github.com/bitcoin/bitcoin/blob/f8462a6d2794be728cf8550f45d19a354aae59cf/test/functional/feature_cltv.py#L26-L35
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
Can be reviewed with --ignore-all-space --color-moved=dimmed-zebra
Co-authored-by: Xekyo <murch@murch.one>
The documentation did not match the implementation, no coins
were mined to the OP_TRUE address.
Fix "Disable GCC suggest-override warnings for proxy clients" https://github.com/chaincodelabs/libmultiprocess/pull/40 is needed to prevent cirrus GCC failure https://cirrus-ci.com/task/6000489311502336?command=ci#L4294
This also includes other recent changes
https://github.com/chaincodelabs/libmultiprocess/pull/35 Fix README.md markdown
https://github.com/chaincodelabs/libmultiprocess/pull/37 Add "make check" target to build and run tests
https://github.com/chaincodelabs/libmultiprocess/pull/38 Add "extends" inherited method support
https://github.com/chaincodelabs/libmultiprocess/pull/41 Avoid depending on argument default constructors
https://github.com/chaincodelabs/libmultiprocess/pull/42 Support attaching custom cleanup functions to proxy client and server classes
https://github.com/chaincodelabs/libmultiprocess/pull/43 Drop hardcoded #include lines in generated files
Add bitcoin-node startup code to let it spawn and be spawned by other
processes
615965cfd1ef1e0627d69970d99bdfedb9176833 Move common package version code to init/common (Russell Yanofsky)
5bed2ab42c4f1a820468f7005ce62e39001f6611 Move common logging start code to init/common (Russell Yanofsky)
1fb7fcfa52569a652d3ea55c210b725e60b7d86f Move common logging GetArgs code to init/common (Russell Yanofsky)
90469c16906ab451bb1250df5e51563870a7ef3b Move common logging AddArg code to init/common (Russell Yanofsky)
387c4cf5887bfdaf1606e1b287d901e4c449514f Move common sanity check code to init/common (Russell Yanofsky)
a67b54855b294802d52f09fa60d3f63550cbada7 Move common global init code to init/common (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This change is move-only and can be easily reviewed with `--color-moved=dimmed_zebra`. The moves are needed to avoid duplicating common init code between different binaries (`bitcoin-node`, `bitcoin-wallet`, etc) in #10102. In #10102, each binary has it's own init file (`src/init/bitcoin-node.cpp`, `src/init/bitcoin-wallet.cpp`) so this PR moves the common code to `src/init/common.cpp`.
ACKs for top commit:
MarcoFalke:
review ACK 615965cfd1ef1e0627d69970d99bdfedb9176833 🖱
practicalswift:
cr ACK 615965cfd1ef1e0627d69970d99bdfedb9176833: dimmed zebra looks correct
Tree-SHA512: 859e1d86aee17eb50a49d806cf62d30d12f6b15018e41c096da41d7e535a9d2d088481cb340fee59e6c68e512a74b61c7146f2683465f553dc4953bf32f2a7b4
Add simple interfaces::Echo IPC interface with one method that just takes and
returns a string, to test multiprocess framework and provide an example of how
it can be used to spawn and call between processes.
To prepare VerifyDB semantics for multiple
chainstate use.
Removes assumptions of use only on the active chainstate.
8c8237a4a10feb2ac9ce46f67b5d14bf879b670f net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d807a1eea5a7c24ae0fe27dc913b1bd net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d1103cd6c25daf07afdf4e65febca6d3f7 net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov)
Pull request description:
This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`.
This change makes the explicit locking of recursive mutexes in the explicit order redundant.
ACKs for top commit:
jnewbery:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
vasild:
ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
ajtowns:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound
MarcoFalke:
review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢
Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
5f438d66c1fbc0e524d12fef233f2ed2952e6f17 refactor, qt: Simplify SendCoinsDialog::updateCoinControlState (João Barbosa)
Pull request description:
This PR doesn't change behaviour, removes the coin control argument from `updateCoinControlState` since it's a class member.
ACKs for top commit:
hebasto:
ACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17, I have reviewed the code and it looks OK, I agree it can be merged.
jonatack:
Code review ACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17
kristapsk:
utACK 5f438d66c1fbc0e524d12fef233f2ed2952e6f17. Code looks correct.
Tree-SHA512: 14abaa3d561f8c8854fed989b6aca886dcca42135880bac76070043f61c0042ec8967f2b83e50bbbb82050ef0f074209e97fa300cb4dc51ee182316e0846506d
a732ee353c1922a1f9ca082775884d190893e0e9 [test] Add tests for addr relay in -blocksonly mode (Amiti Uttarwar)
a6694eaed8f86a6ad79689bbcbfdbed4d73327c9 [test] Add address relay tests involving outbound peers (Martin Zumsande)
8188b77c17d78bf608cb41978db4d0f46e91f7d8 [test] Add tests for getaddr behavior (Martin Zumsande)
d2dbfe6ff1806e5248399b28a7a1a22dab726c40 [test] Extract sending an addr message into a helper (Amiti Uttarwar)
c9919433994924931757aa7ec278c50744ede35d [test] Refactor the addr relay test to prepare for new tests (Amiti Uttarwar)
Pull request description:
This extends the functional test `p2p_addr_relay.py`.
It adds test coverage for address relay involving outbound peers, tests for both outgoing and incoming `GETADDR` requests and tests for `-blocksonly` mode.
The initial refactors and some of the new tests were taken from Amiti Uttarwar's PR #21528 - they are general test improvements not directly tied to the change proposed there.
ACKs for top commit:
amitiuttarwar:
re-ACK a732ee353c, small diff based on code review
MarcoFalke:
Concept ACK a732ee353c1922a1f9ca082775884d190893e0e9 🌊
Tree-SHA512: e80d52683808ddd6b948a5134239f002f3fecf61b60e187877b07be6251721fde847104e495c75a1a5133a09c0b41a9255a0bec82932c0b304b516fa89bce33e
c5a470eee1ac864b7c02b6a1669327b68411d806 refactor: Drop CCoinControl::SetNull (João Barbosa)
Pull request description:
The only external call to `SetNull` is changed as follow
```diff
- m_coin_control->SetNull();
+ m_coin_control = std::make_unique<CCoinControl>();
```
ACKs for top commit:
theStack:
Code-Review ACK c5a470eee1ac864b7c02b6a1669327b68411d806
MarcoFalke:
review ACK c5a470eee1ac864b7c02b6a1669327b68411d806 🍤
Tree-SHA512: 2588828720cdcf405fc4fb06f2aa17ad4eef2a645e12d820311006127e732258dd084993f17f23742f8e7f782e18289a6199dcec3690efc9b92458f90b816a8f
fa4aec2b26696cc16dc44c6425f7dca3ef91c8ee test: Fix feature_notifications.py intermittent issue (MarcoFalke)
Pull request description:
Fixes #21683
Top commit has no ACKs.
Tree-SHA512: 256806d82877477f4b3d795658f61127c0de4eff07216f6071f40a8ec1f5d43f3c587f35dd436d480dc261ef6646ac5547db104d22f3fcfeeb61bbdbe04bcc31
Co-authored-by: Xekyo <murch@murch.one>
Co-authored-by: Xekyo <murch@murch.one>
785f9cc46a43661c63a5ec56a9e82f4ce9d42f44 refactor: init: mark fReset const (James O'Beirne)
Pull request description:
Small thing, but hey - it doesn't change.
ACKs for top commit:
theStack:
Code-review ACK 785f9cc46a43661c63a5ec56a9e82f4ce9d42f44
Tree-SHA512: 3cb8d7037f517162f6315d561accc4932b0f1e340162c3283871433f2e355d57b3740c9d2e953ce33fbfa3b277c8437f91955fb70331b3fe9c8e6a8589dc2b49
d831e711cab83c70bf2ded62fe33f484844e73dd [validation] RewindBlockIndex no longer needed (Dhruv Mehta)
Pull request description:
Closes #17862
Context from [original comment](https://github.com/bitcoin/bitcoin/issues/17862#issuecomment-744285188) (minor edits):
`RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows:
- A pre-segwit (i.e. v0.13.0 or older) node is running.
- Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules.
- The user upgrades the node to a segwit-aware version (v0.13.1 or newer).
- On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules.
- those blocks are then redownloaded (with witness data) and validated with segwit rules.
This logic probably isn't required any more since:
- Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested.
- Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22.
This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data.
Removing this code allows the following (done in follow-up #21090):
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
jnewbery:
utACK d831e711cab83c70bf2ded62fe33f484844e73dd
jamesob:
ACK https://github.com/bitcoin/bitcoin/pull/21009/commits/d831e711cab83c70bf2ded62fe33f484844e73dd
laanwj:
Cursory code review ACK d831e711cab83c70bf2ded62fe33f484844e73dd. Agree with the direction of the change, thanks for simplifying the logic here.
glozow:
utACK d831e711cab83c70bf2ded62fe33f484844e73dd
Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053
84934bf70e11fe4cda1cfda60113a54895d4fdd5 multiprocess: Add echoipc RPC method and test (Russell Yanofsky)
7d76cf667eff512043a28d4407cc89f58796c42b multiprocess: Add comments and documentation (Russell Yanofsky)
ddf7ecc8dfc64cf121099fb047e1ac871de94f4c multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky)
10afdf0280fa93bfffb0a7665c60dc155cd84514 multiprocess: Add Ipc interface implementation (Russell Yanofsky)
745c9cebd50fea1664efef571dc1ee1bddc96102 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky)
5d62d7f6cd48bbc4e9f37ecc369f38d5e1e0036c Update libmultiprocess library (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*].
These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.
These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."
Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.
Additional notes about this PR can be found at https://bitcoincore.reviews/19160
[*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md)
ACKs for top commit:
fjahr:
re-ACK 84934bf70e11fe4cda1cfda60113a54895d4fdd5
ariard:
ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`.
Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6
844ad0eccaa1dbfefc30d19804d95d67c3d5f06d doc: IsSnapshotActive (James O'Beirne)
9b604c0207b526c008617cdca210f35db5e344db validation: prepare VerifyDB for assumeutxo (James O'Beirne)
7901647d722bcc5b0554ad73b14481cbe73608e4 refactor: rename active_chainstate in VerifyDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
~~Pretty cut and dry; parameterizes `CVerifyDB` methods so that we can run the verify procedure on multiple chainstates.~~
Two minor tweaks to ensure that `VerifyDB` can be run on multiple chainstates and a corresponding rename.
ACKs for top commit:
fjahr:
Code review re-ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d
MarcoFalke:
review ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d 🐥
Tree-SHA512: 26a398cf4dabc1aa0850743921dba0452b4813848a3c777586dc981716737e98e17b8110254a5c41af95dd236e0c00dc8b4eee891d69bef825a5e1911fc499d0
fa1fdeb2306ab86eb0ae7e8b9764bc37b1b5f04b fuzz: Ensure prevout is consensus-valid (MarcoFalke)
Pull request description:
It shouldn't be possible to create consensus-invalid prevouts, so there is no need to fuzz them.
To reproduce:
```
$ echo 'AAAAAAEAAAAAAQF0Rw0SGsrit4+YZSEfpcQT/o+bJbjgVjATUHqrCfRE+QsBAAAXFgAUlsvXHgGV
ZxF3QXxitwe1tIOYdLj2NePHATl9CgAAAAAAGXapFOFHg1yqRFl7soeowwpIEOoe9G1NiKwCRzBE
AiAx6F2Q008gvJnok6JiyOn7lPqCJJmDiI2omRNXT1Q7XAIgCQP6WJizAqhnvImpQqYMJkqePGvx
Jy/pGRMy1iNL0ecDIQJr4tWomVTBfjpyMFMOD9aDAR5gkByOIYiaQOv8P/sRztP3pS8RDAAAEUUE
NQBwYAAAAAC5F6kUTLIzj/lKP2Hmpwyzukns2eweRkOH' | base64 --decode > /tmp/a
$ FUZZ=script_flags ./src/test/fuzz/fuzz /tmp/a
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 59714236
INFO: Loaded 1 modules (212532 inline 8-bit counters): 212532 [0x55987fb3f668, 0x55987fb7349c),
INFO: Loaded 1 PC tables (212532 PCs): 212532 [0x55987fb734a0,0x55987feb17e0),
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: /tmp/a
fuzz: script/interpreter.cpp:1495: bool HandleMissingData(MissingDataBehavior): Assertion `!"Missing data"' failed.
==520092== ERROR: libFuzzer: deadly signal
#0 0x55987f111180 in __sanitizer_print_stack_trace (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5ea180)
#1 0x55987f0ba828 in fuzzer::PrintStackTrace() fuzzer.o
#2 0x55987f09de43 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
#3 0x7fd003d563bf (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
#4 0x7fd00399a18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
#5 0x7fd003979858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
#6 0x7fd003979728 (/lib/x86_64-linux-gnu/libc.so.6+0x25728)
#7 0x7fd00398af35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35)
#8 0x55987f8ce194 in HandleMissingData(MissingDataBehavior) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1495:9
#9 0x55987f8ce194 in GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1685:68
#10 0x55987f8cbbc8 in EvalChecksigPreTapscript(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, prevector<28u, unsigned char, unsigned int, int>::const_iterator, prevector<28u, unsigned char, unsigned int, int>::const_iterator, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptError_t*, bool&) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:363:24
#11 0x55987f8cbbc8 in EvalChecksig(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, prevector<28u, unsigned char, unsigned int, int>::const_iterator, prevector<28u, unsigned char, unsigned int, int>::const_iterator, ScriptExecutionData&, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptError_t*, bool&) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:421:16
#12 0x55987f8c5a01 in EvalScript(std::vector<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >&, CScript const&, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptExecutionData&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1094:26
#13 0x55987f8d6d6e in ExecuteWitnessScript(Span<std::vector<unsigned char, std::allocator<unsigned char> > const> const&, CScript const&, unsigned int, SigVersion, BaseSignatureChecker const&, ScriptExecutionData&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1843:10
#14 0x55987f8d48fc in VerifyWitnessProgram(CScriptWitness const&, int, std::vector<unsigned char, std::allocator<unsigned char> > const&, unsigned int, BaseSignatureChecker const&, ScriptError_t*, bool) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1904:20
#15 0x55987f8d3d8b in VerifyScript(CScript const&, CScript const&, CScriptWitness const*, unsigned int, BaseSignatureChecker const&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:2045:22
#16 0x55987f201d47 in script_flags_fuzz_target(Span<unsigned char const>) /root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/script_flags.cpp:54:30
#17 0x55987f11447f in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
#18 0x55987f8aed17 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
#19 0x55987f8aed17 in LLVMFuzzerTestOneInput /root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz.cpp:63:5
#20 0x55987f09f5e3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
#21 0x55987f0894e2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
#22 0x55987f08f2da in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
#23 0x55987f0bb002 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x594002)
#24 0x7fd00397b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#25 0x55987f06420d in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x53d20d)
NOTE: libFuzzer has rudimentary signal handlers.
Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
ACKs for top commit:
practicalswift:
Tested ACK fa1fdeb2306ab86eb0ae7e8b9764bc37b1b5f04b
Tree-SHA512: 6130ed9ab6d8eeab901f64a1c069300e67d0b6009c42763262fe6edeab8192e088c1a3c1f61aee900b9ebbc48fbf6e837b41704bad592ec526398355766e208a
363df758a9e0b5e0aa5ab401de2de820fc362c19 doc/style followups in MempoolAcceptResult (glozow)
Pull request description:
Follow up to #21062. Was going to be a part of #20833 but I'm trying to break it down as much as possible.
- Make members const (https://github.com/bitcoin/bitcoin/pull/21062#discussion_r573659273)
- List fee units (https://github.com/bitcoin/bitcoin/pull/21062#discussion_r569329362)
- Use default value for `TxValidationState` in the success case (https://github.com/bitcoin/bitcoin/pull/21062#discussion_r573659801).
ACKs for top commit:
jnewbery:
ACK 363df758a9e0b5e0aa5ab401de2de820fc362c19
practicalswift:
cr ACK 363df758a9e0b5e0aa5ab401de2de820fc362c19: patch looks correct and `const` is better than non-`const` (where possible :))
ariard:
Code Review ACK 363df75
Tree-SHA512: 0ff1a0279e08e03204e48d0f4c92428d7f39c32f52c1d20fe6a0283d605839898297344be82ca69640ba9f878ca4ebd5da2d717e26d719a183b211d709334082
545404e7e1c72985557ccffe865cea269143e5dd fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. (practicalswift)
Pull request description:
Add RPC interface fuzzing.
This PR increases overall fuzzing line coverage from [~65%](https://marcofalke.github.io/btc_cov/fuzz.coverage/) to ~70% 🎉
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make -C src/ test/fuzz/fuzz
$ FUZZ=rpc src/test/fuzz/fuzz
```
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for more information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
Concept ACK 545404e7e1c72985557ccffe865cea269143e5dd
Tree-SHA512: 35fc1b508af42bf480ee3762326b09ff2eecdb7960a1917ad16345fadd5c0c21d666dafe736176e5a848ff6492483c782e4ea914cd9000faf50190df051950fd
a29f522ba4aa71582b54025c5682b4c1687ae9f3 fuzz: bolster ExtractDestination(s) checks (Michael Dietz)
Pull request description:
ACKs for top commit:
practicalswift:
Tested ACK a29f522ba4aa71582b54025c5682b4c1687ae9f3
Tree-SHA512: 0fc194edb7b0fce77c7bb725fe65dec7976598edcd53882b5a0eb7cd83281a3ddcd2b3de00282468be659a7e5bc9991eb482816418f55b30e657cdc5a3bd7438
5a4a15d2b4456272fd8aa080195f40a09576ae01 qt, refactor: Drop no longer used PeerTableModel::getRowByNodeId func (Hennadii Stepanov)
9a9f180df0d51396fee2468681df6dd935b0248e qt, refactor: Drop no longer used PeerTableModel::sort function (Hennadii Stepanov)
778a64af209e4fa692a3aca8376ba1bd5e1af881 qt: Use PeerTableSortProxy for sorting peer table (Hennadii Stepanov)
df2d165ba9e0acc53f36a326f68f57ad9c297872 qt: Add peertablesortproxy module (Hennadii Stepanov)
Pull request description:
The "Peers" table in the "Node" window does not hold multiple selection after sorting.
This PR introduces a `QSortFilterProxyModel` subclass, that is a standard Qt [practice](https://doc.qt.io/qt-5/model-view-programming.html#custom-sorting-models) for such cases.
Now the sorting code is encapsulated into the dedicated Qt class, and we do not need to maintain it.
Fixes #283 (additionally).
---
On **master** (7ae86b3c6845873ca96650fc69beb4ae5285c801):
- rows are sorted by "Ping", and a selection is made
- rows are sorted by "NodeId", and the previous selection is _lost_
With **this PR**:
- rows are sorted by "Ping", and a selection is made
- rows are sorted by "NodeId", and the row are still selected
ACKs for top commit:
jarolrod:
re-ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01, tested on macOS 11.2 Qt 5.15.2 after rebase
promag:
Tested ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01.
Tree-SHA512: f81c1385892fbf1a46ffb98b42094ca1cc97da52114bbbc94fedb553899b1f18c26a349e186bba6e27922a89426bd61e8bc88b1f7832512dbe211b5f834e076e
83a425d25af033086744c1c8c892015014ed46bd compressor: use a prevector in compressed script serialization (William Casarin)
Pull request description:
This function was doing millions of unnecessary heap allocations during IBD.
I'm start to catalog unnecessary heap allocations as a pet project of mine: as-zero-as-possible-alloc IBD. This is one small step.
before:
after:
~should I type alias this?~ *I type aliased it*
This is a part of the Zero Allocations Project #18849 (ZAP1). This code came up as a place where many allocations occur.
ACKs for top commit:
Empact:
ACK https://github.com/bitcoin/bitcoin/pull/18847/commits/83a425d25af033086744c1c8c892015014ed46bd
elichai:
tACK 83a425d25af033086744c1c8c892015014ed46bd
sipa:
utACK 83a425d25af033086744c1c8c892015014ed46bd
Tree-SHA512: f0ffa6ab0ea1632715b0b76362753f9f6935f05cdcc80d85566774401155a3c57ad45a687942a1806d3503858f0bb698da9243746c8e2edb8fdf13611235b0e0
b353633bf488fbd89b66f6c534d5f0f676c9cf6d build: mac_alias 2.2.0 (sgulls)
Pull request description:
Fix make deploy for arm64-darwin
Accidentally [closed](https://github.com/bitcoin/bitcoin/pull/21555) the PR
ACKs for top commit:
promag:
Tested ACK b353633bf488fbd89b66f6c534d5f0f676c9cf6d.
Tree-SHA512: 08043792d63894b6738ea93d076cecace1d8b30a623b944170a34492c3838269da87e09878164c760cf321663fb72641a7295070a847ad67d91fc9970ebe5c6a
faa51ef4d33034f8c14cfd18f1c01c90883a1bb4 test: Fix intermittent issue in p2p_addr_relay.py (MarcoFalke)
fa37116c820f6a84642d57ce709a5977fd5616c4 test: Use self.send_addr_msg (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/21707/files#r621048952
ACKs for top commit:
mzumsande:
ACK faa51ef4d33034f8c14cfd18f1c01c90883a1bb4
amitiuttarwar:
ACK faa51ef4d33034f8c14cfd18f1c01c90883a1bb4
Tree-SHA512: b1fbc1a4a13b020890b465083a82762981de17dd710388c651353aefc28dee23d39803c2a1dadbf88287e92598901afcb01b488115b0fecd746ab6d73775415b
fa40eb5b6bfd151912c58d61771f6a6528f44e67 test: Speed up mempool_spend_coinbase.py (MarcoFalke)
fa29382ab23d52b86bfda8a267195b6c51b713c2 test: Fix test cache issue (MarcoFalke)
fa085b470a9647f3b261f506b46f4e7ca2baf0b0 test: Create MiniWallet.create_self_transfer (MarcoFalke)
fa1bedb4944b513a3c9184ad549f58bfbe69e20e test: Add MiniWallet.sendrawtransaction (MarcoFalke)
Pull request description:
Locally the test will run 4 seconds faster with `--valgrind` (18s vs 14s)
ACKs for top commit:
mjdietzx:
crACK https://github.com/bitcoin/bitcoin/pull/21762/commits/fa40eb5b6bfd151912c58d61771f6a6528f44e67
Tree-SHA512: ecfb60dda5ca5d7e6367bb9c6210390d95ebf6396ce657728901d118b75bb90c98f9351df3b01004d00682234448d6c6a13338d12097f7dced2cf7f1bd84d924
Never sleep for more than 5 seconds when waiting for an
inv-getdata exchange to time out.
fa00bb2c5ca64c7eb9e1846ffedc7829859812ca test: Add missing shift-base:nanobench.h suppression (MarcoFalke)
00004565ccdbaf6bf337e10a5f5ae463bd0ccf9a ci: Use clang-12 for asan task (MarcoFalke)
Pull request description:
ACKs for top commit:
fanquake:
ACK fa00bb2c5ca64c7eb9e1846ffedc7829859812ca
Tree-SHA512: fe7cd1ad9f3e73c09f7f84dfb0f276d0cda603c4d591b9338a0914bf1276b0247fd2faee7052f5962c3ae3280e7fa8b72f5b773b84c2a8882a89ed1f8c08256c
a33bdb52d192d3e1f4697c98d0e80b2fa2bbe53a [tests] Speed up p2p_segwit.py (John Newbery)
Pull request description:
Never sleep for more than 5 seconds when waiting for an
inv-getdata exchange to time out.
Shaves about 1 minute of the runtime of p2p_segwit.py.
ACKs for top commit:
MarcoFalke:
review ACK a33bdb52d192d3e1f4697c98d0e80b2fa2bbe53a 🐳
Tree-SHA512: 7bd892ed0b1b817579f88910ba4714519bd0d871241e1b9a67968d297de1ed63d558115abad2aae4d105ff176c35a7079a3a789f3053442aed30d6e1aefb5c4a
5e7d1997fce15b3e2fb77e67491237812e1f98d9 doc: note on SDK for macOS depends cross-compile (Jarol Rodriguez)
Pull request description:
This PR adds a friendly note for those who are unfamiliar with our depends build system that an extracted SDK is needed before proceeding with a macOS cross-compile. Additionally, it refers a builder to look at [macdeploy](https://github.com/bitcoin/bitcoin/tree/master/contrib/macdeploy#sdk-extraction) for instructions on obtaining the SDK and provides context to where this SDK should reside.
**Master:** [render](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md#for-macos-cross-compilation)
**PR:** [render](https://github.com/bitcoin/bitcoin/blob/241c4017eddbe33a39073766accf556c2cad90a0/depends/README.md#for-macos-cross-compilation)
ACKs for top commit:
jonatack:
ACK 5e7d1997fce15b3e2fb77e67491237812e1f98d9
Sjors:
ACK 5e7d199
hebasto:
re-ACK 5e7d1997fce15b3e2fb77e67491237812e1f98d9
Tree-SHA512: bbdff4f6ea1dfae03bca7fd30dfba543c4d130960075195e42695875382f13b93ea02128f80686fa4d1b5c3bd3703ac600f8ba07356ea0caf0fec09b7dbd081e
6ba892126d354219b146f0c7f35d472f9c14bdac refactor + document coin selection strategy (glozow)
58ea324fdd906204bb77ea4be1c01a3ab56cf86f [docs] add doxygen comments to wallet code (glozow)
0c74716c50384677724247e05e6592f845fc8635 [docs] format existing comments as doxygen (glozow)
Pull request description:
I think it would help code review to have more documentation + doxygen comments
ACKs for top commit:
Xekyo:
ReACK https://github.com/bitcoin/bitcoin/commit/6ba892126d354219b146f0c7f35d472f9c14bdac
achow101:
ACK 6ba892126d354219b146f0c7f35d472f9c14bdac
Tree-SHA512: 74a78d9b0e0c1d5659bed566432a5b3511511d8b2432f440565f443da7b8257a1b90e70aa7505a7f8abf618748eeb43d166e84f278bdee3d34ce5d5c37dc573a
fad6269916dbf8adc14d757a18f19c74e95cf659 test: Assert that exit code indicates failure (MarcoFalke)
faecb72c3ca744f1adb77bd910c643cedec3b445 test: Fix intermittent issue in p2p_segwit.py (MarcoFalke)
Pull request description:
Calling `start_node` might call `wait_for_rpc_connection`, which will fail.
https://cirrus-ci.com/task/5669555591708672?logs=ci#L3504
```
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_segwit.py", line 1974, in test_upgrade_after_activation
self.start_node(2, extra_args=["-reindex", f"-segwitheight={SEGWIT_HEIGHT}"])
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 508, in start_node
node.wait_for_rpc_connection()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status 1 during initialization
ACKs for top commit:
jnewbery:
ACK fad6269916dbf8adc14d757a18f19c74e95cf659
dhruv:
ACK fad6269
Tree-SHA512: 4c5e39ce25e135717ea433258518f93f09d1c528c4538a8627d3da13bc0c0ba4b45911703c26392ff0f5e0cb7831a6c7cc53e6e29102d3da9c8cfce7cef333cc
9bd3f35003c2e9eff74766d57a71d6b391ab602a build: adds switch for disabling random base addresses in MSVC (Ethan Heilman)
Pull request description:
In m4 builds we have the --disable-hardening switch that can be given in `./configure` to turn off randomized addresses. This PR provides a simple way of turning off randomized addresses in MSVC builds.
This PR:
* Adds this option the common-init project file so that it can be globally set across the project
* Documents this switch in msvc build readme
I have run the following test to verify this works
I ran the msvc build with `<RandomizedBaseAddress>true</RandomizedBaseAddress>` then checked `bitcoind.exe` and `bitcoin-cli.exe` with `dumpbin.exe`:
bitcoind
```
> .\dumpbin.exe /headers src/bitcoind.exe
Microsoft (R) COFF/PE Dumper Version 14.16.27045.0
...
OPTIONAL HEADER VALUES
20B magic # (PE32+)
14.28 linker version
AE4600 size of code
345C00 size of initialized data
0 size of uninitialized data
6BED74 entry point (00000001406BED74) mainCRTStartup
1000 base of code
140000000 image base (0000000140000000 to 0000000140E2DFFF)
1000 section alignment
200 file alignment
6.00 operating system version
0.00 image version
6.00 subsystem version
0 Win32 version
E2E000 size of image
400 size of headers
0 checksum
3 subsystem (Windows CUI)
8160 DLL characteristics
High Entropy Virtual Addresses
Dynamic base
NX compatible
Terminal Server Aware
```
bitcoin-cli
```
> .\dumpbin.exe /headers src/bitcoin-cli.exe
Microsoft (R) COFF/PE Dumper Version 14.16.27045.0
...
OPTIONAL HEADER VALUES
20B magic # (PE32+)
14.28 linker version
1E3E00 size of code
92C00 size of initialized data
0 size of uninitialized data
104384 entry point (0000000140104384) mainCRTStartup
1000 base of code
140000000 image base (0000000140000000 to 0000000140279FFF)
1000 section alignment
200 file alignment
6.00 operating system version
0.00 image version
6.00 subsystem version
0 Win32 version
27A000 size of image
400 size of headers
0 checksum
3 subsystem (Windows CUI)
8160 DLL characteristics
High Entropy Virtual Addresses
Dynamic base
NX compatible
Terminal Server Aware
```
Then I built with `<RandomizedBaseAddress>false</RandomizedBaseAddress>` then checked `bitcoind.exe` and `bitcoin-cli.exe` with `dumpbin.exe` and observed that `Dynamic base` was longer listed in `OPTIONAL HEADER VALUES`
bitcoind
```
PS C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.16.27023\bin\HostX64\x64> .\dumpbin.exe /headers C:\Users\e0\Documents\GitHub\bitcoin021noaslr/src/bitcoind.exe
Microsoft (R) COFF/PE Dumper Version 14.16.27045.0
...
OPTIONAL HEADER VALUES
20B magic # (PE32+)
14.28 linker version
AE4600 size of code
33FE00 size of initialized data
0 size of uninitialized data
6BED74 entry point (00000001406BED74) mainCRTStartup
1000 base of code
140000000 image base (0000000140000000 to 0000000140E27FFF)
1000 section alignment
200 file alignment
6.00 operating system version
0.00 image version
6.00 subsystem version
0 Win32 version
E28000 size of image
400 size of headers
0 checksum
3 subsystem (Windows CUI)
8120 DLL characteristics
High Entropy Virtual Addresses
NX compatible
Terminal Server Aware
```
bitcoin-cli
```
> .\dumpbin.exe /headers src/bitcoin-cli.exe
Microsoft (R) COFF/PE Dumper Version 14.16.27045.0
...
OPTIONAL HEADER VALUES
20B magic # (PE32+)
14.28 linker version
1E3E00 size of code
90C00 size of initialized data
0 size of uninitialized data
104384 entry point (0000000140104384) mainCRTStartup
1000 base of code
140000000 image base (0000000140000000 to 0000000140277FFF)
1000 section alignment
200 file alignment
6.00 operating system version
0.00 image version
6.00 subsystem version
0 Win32 version
278000 size of image
400 size of headers
0 checksum
3 subsystem (Windows CUI)
8120 DLL characteristics
High Entropy Virtual Addresses
NX compatible
Terminal Server Aware
```
ACKs for top commit:
sipsorcery:
ACK 9bd3f35003c2e9eff74766d57a71d6b391ab602a.
practicalswift:
cr ACK 9bd3f35003c2e9eff74766d57a71d6b391ab602a: patch looks correct
Tree-SHA512: ddffdb4ff8a09c7cfef61c07a5db2a2828e9e3aa795ad8e5a1bf51ab489a68b40f87f6694518c5e0b8858c0fad4f93bb947b052e6b9d5e55eb38e764b746fc02
415fb2e1abac189fcbe8eccf2ea065724d17460f GUI/Intro: Move prune setting below explanation (Luke Dashjr)
2a84c6bcf61429ac507b506a39247b3a66edbcb4 GUI/Intro: Estimate max age of backups that can be restored with pruning (Luke Dashjr)
e2dcd957fa206a28404ff824fb764b8889705fb2 GUI/Intro: Rework UI flow to let the user set prune size in GBs (Luke Dashjr)
f2e5a6b54fa38b10d822609939b8108bd8fe041b GUI/Intro: Abstract GUI-to-option into Intro::getPrune (Luke Dashjr)
62932cc686dc46ce14c026993deea8e561654177 GUI/Intro: Return actual prune setting from showIfNeeded (Luke Dashjr)
Pull request description:
Moved from https://github.com/bitcoin/bitcoin/pull/18728
ACKs for top commit:
ryanofsky:
Code review ACK 415fb2e1abac189fcbe8eccf2ea065724d17460f. Changes since last review: mb/gib suffixes, constexpr QOverload expected_backup_days tweaks, new moveonly layout commit
jarolrod:
Tested ACK 415fb2e.
Talkless:
tACK 415fb2e1abac189fcbe8eccf2ea065724d17460f, tested on Debian Sid with Qt 5.15.2.
hebasto:
ACK 415fb2e1abac189fcbe8eccf2ea065724d17460f, my unresolved comments are not blockers, and they could be resolved in follow ups.
Tree-SHA512: bd4882a9c08e6a6eb14b7fb6366983db8581425b4949fea212785d34d8fad9e32fb81ca8c8cdbfb2c05ea394aaf5a746ba2cf16623795c7252c3bdb61d455f00
61fd8fe9a1ddb866ad0a41d053003e78c395f739 Remove progress bar on modal overlay (bruno)
Pull request description:
This PR removes the progress bar (keeping only the percentage) on modal overlay
resolves #279
Before:
After:
ACKs for top commit:
Bosch-0:
tACK https://github.com/bitcoin-core/gui/pull/300/commits/61fd8fe9a1ddb866ad0a41d053003e78c395f739 on Windows 10. Unnecessary Progress bar no longer there :)
jarolrod:
tACK 61fd8fe9a1ddb866ad0a41d053003e78c395f739
Tree-SHA512: 96d72f168b26e950ce37e9f489bcbcc608473c44bce3be127ccd47d17b7642fa234d314596186ee16b430d943575c312d84133425507a17ae7ac58ecae986639
5f96d7d22d8e05876c6fc014e70488699950fe38 rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436be641d7417152adc683192649ba206a test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3cfbd6bce5883f46d8527c6853a86a2 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392aef2689bc106c20925859ede555d082b index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121a30489bc81a1f46dde6a9b19ae4ec1 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c29099635150014ffc9bb0cafa8049ec55a test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576eccb08af80471c7b7b843b189ad6758c0 rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836a71af23d2035d2e2e99ad8b8c340c3 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d689f93e1c7669b0ba7a4994c0206dabd rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30fef3138bb8db46e6865acb9dc2674f8 test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab4d073a26c3c4a02783d5dcd88f6eef test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc125fce6ccd995687fa16572090a5d099 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff323255b32e717d0ce28209ec0abdaa index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21469d6d848ae309edc47f558628221d index: Add Coinstats index (Fabian Jahr)
a8a46c4b3cfda4b95c92a36f8cebd3606377e57d refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd21a87228c18a1661df99fedc1866baf refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a9021dfbb6e17dfa81472f057dacbc34e0 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d8e05876c6fc014e70488699950fe38
jonatack:
Code review re-ACK 5f96d7d22d8e05876c6fc014e70488699950fe38 per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d8e05876c6fc014e70488699950fe38. Light code review ACK 5f96d7d22d8e05876c6fc014e70488699950fe38.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
e94920a0bb859d557dd978febde0a65d46fabb68 qt: peertableview alternating row colors (randymcmillan)
Pull request description:
peers-tab: enable alternating row colors for peer table and banned table
ACKs for top commit:
Bosch-0:
tACK https://github.com/bitcoin-core/gui/commit/e94920a0bb859d557dd978febde0a65d46fabb68 on Windows 10 - works as intended. Before / after below:
jarolrod:
tACK e94920a0bb859d557dd978febde0a65d46fabb68
Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
cf971c9ea0e8637d36a7ab8b7b8804d9b89f1d9a build: use -isysroot over --sysroot on macOS (fanquake)
Pull request description:
Not only does this seem to be the more correct behaviour when targeting Darwin, but if you use `-isysroot`, Clangs Darwin driver will [infer the deployment target](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1652) from the SDK and use other SDK info when parsing arguments to the linker. In the case of [`-platform_version`](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L2656), which is added if the linker is [new enough](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L342), the version tuple is constructed from the SDKInfo, and SDKInfo, as far as I can tell, only exists when `-isysroot` has been passed, see [parseSDKSettings](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1785)
As a result, the SDK version field in the `LC_BUILD_VERSION` command is filled out. i.e when building master:
```bash
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk n/a
ntools 1
tool 3
version 650.9
```
vs this PR:
```bash
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk 11.3
ntools 1
tool 3
version 650.9
```
This, from what I understand, will fix the issue we are having with Qt deciding wether or not to enable features like "Dark mode" on macOS, see #21771, however I have not tested that. Thus this is an alternative to #21782.
Our usage of `--sysroot` was added in #17118.
```bash
-isysroot <dir> Set the system root directory (usually /)
```
ACKs for top commit:
Sjors:
tACK cf971c9ea0e8637d36a7ab8b7b8804d9b89f1d9a
hebasto:
re-ACK cf971c9ea0e8637d36a7ab8b7b8804d9b89f1d9a, only rebased and addressed comments since my [previous](https://github.com/bitcoin/bitcoin/pull/21793#pullrequestreview-647321518) review.
Tree-SHA512: f01138179fb85083b5505bbaa48810451098ffa4da5d3c9b673785448790aa76f2e64b2aab6e698f6ee378a21f70626445a3fabee7c61dbfc44e96f3e3964656
c5bb142817c53c6a217163958b5d511f12171004 test: resolve bug in test/functional/interface_bitcoin_cli.py - Test -getinfo with -rpcwallet=unloaded wallet returns no balances (klementtan)
Pull request description:
I think there is a bug in this test case where the new value of `cli_get_info` is not asserted.
ACKs for top commit:
jonatack:
ACK c5bb142817c53c6a217163958b5d511f12171004
Tree-SHA512: 50c0c2c8fe63c95f951dee892fbacedf92208f47efe5ed481fbb255f15137c799d9200fa3ff31a442df0691248d7ff04d899842722c3032cd7f35553622ba38c
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017.
When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used.
It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious).
Added advisory note about build not working with earlier Visual Studio versions.
Fixed grammar.
0a331456e44ce2bdd3bfe62f7603a312905c6624 Remove Visual Studio 2017 reference from readme (Aaron Clauson)
Pull request description:
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017.
When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used.
It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious).
ACKs for top commit:
jarolrod:
ACK 0a331456e44ce2bdd3bfe62f7603a312905c6624
hebasto:
ACK 0a331456e44ce2bdd3bfe62f7603a312905c6624, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 17e2326cd55a5cc3fc13622ba72bb44f9b9d55070cb77941df5fd943cd3f1baf02b9ac9504dfae5941c522748ad7b584c3c8da03fb323a87b3639eb926ce1699
54133c59b80ccac85eaebb0668cd2f0fe360b323 doc: add indexes/coinstats/db/ to files.md (Jon Atack)
5d1050f51647980b1204e3b44b319ab31948d11f doc: fix -coinstatsindex help, and test/rpc touchups (Jon Atack)
e041ee0a80e5f3e10301acf8512a18864af750cd doc: add coinstatsindex to bitcoin.conf (Jon Atack)
Pull request description:
ACKs for top commit:
Sjors:
utACK 54133c59b80ccac85eaebb0668cd2f0fe360b323
MarcoFalke:
cr ACK 54133c59b80ccac85eaebb0668cd2f0fe360b323
clarkmoody:
utACK 54133c5
Tree-SHA512: 1a7f3e89873b7dc79ec71d5d39e9e3e4977ce43cc4bee208ad55291bef1bb319a9d1c34ed84a87d6a803db983bdfd0af4d9f396cec0bec86b1701ebbb6f34378
fab53ea72fd6c763ddc2627bc83603a1c78d548a doc: Add doc/release-notes/release-notes-0.21.1.md (MarcoFalke)
Pull request description:
ACKs for top commit:
laanwj:
ACK fab53ea72fd6c763ddc2627bc83603a1c78d548a
Tree-SHA512: f228c51c5fd4d18badc72ca185b91a05b248614a69646b234e03963626eccbafcf23c8057bbaa9ae3a3fc4e7d8f007fd69306fc87da229c0c44847de8be42aad