b01cd9471f test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc1981474 test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c48 test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202 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:
f8462a6d27/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 b01cd9471f🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
It is not possible to have a node in `CConnman::vNodesDisconnected` and
its reference count to be incremented - all `CNode::AddRef()` are done
either before the node is added to `CConnman::vNodes` or while holding
`CConnman::cs_vNodes` and the object being in `CConnman::vNodes`.
So, the object being in `CConnman::vNodesDisconnected` and its reference
count being zero means that it is not and will not start to be used by
other threads.
So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will
always succeed and is not necessary.
Indeed all locks of `CNode::cs_vSend` are done either when the reference
count is >0 or under the protection of `CConnman::cs_vNodes` and the
node being in `CConnman::vNodes`.
417305991a 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 417305991a, 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
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
c799a19b4b 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](5283a6c87b).
ACKs for top commit:
fanquake:
ACK c799a19b4b
Tree-SHA512: 9d116ac1e8c605ee3e8ed7f618586f0de85d8b06bbbb70fe8c298939ce203d2a7e97264a9afac037179993ab54c5f69a65ebb9ab27ca7f45acb963011bd45743
7abac98d3e 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 7abac98d3e: patch looks correct
fanquake:
ACK 7abac98d3e
Tree-SHA512: b6a37c1728ec3b2e552f244da0e66db113c1e7662c7ac502e12ff466f3dbfbfefae12695ca135137c50dbb1c4c5d84059116c0cd09b391a17466dc77b8726679
fafb68add5 refactor: Add and use EnsureConnman in rpc code (MarcoFalke)
faabeb854a 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 fafb68add5
theStack:
ACK fafb68add5
ryanofsky:
Code review ACK fafb68add5
Tree-SHA512: 84c63cfe31e548645d906f7191a3526c7bea99ed0d54c2a75c2041452a44fe149ede343d8e1943b0e7770816c828bb047dfec8bc541a1f2b89920a126ee54d68
16c157de3c qt, refactor: Use better QMenu::addAction overloaded function (Hennadii Stepanov)
79311750b5 qt: Do not assign Alt+<KEY> shortcuts to context menu actions (Hennadii Stepanov)
963e12058f qt: Drop menu separator that separates nothing (Hennadii Stepanov)
1398a6536c 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 16c157de3c
promag:
Code review ACK 16c157de3c. Nice code cleanup that takes advantage of more recent Qt API.
jarolrod:
ACK 16c157de3c
Tree-SHA512: e5555fe957058cc67b351aaf9f09fe3635edb2d07a2223d3093913a25607ae538f0a2fde84c0b0cd43e7475b248949548eb4a5d4b21d8f7391fa2fa8541c04ff
35d52397e7 Add bitcoin_en.xlf intermediate translation file to the repo (Hennadii Stepanov)
99686b6519 qt [experimental]: Add a translation comment and a disambiguation string (Hennadii Stepanov)
f959b75e8c build: Add Qt lconvert tool to depends (Hennadii Stepanov)
2045e4cdd2 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: 35d52397e7/src/qt/locale/bitcoin_en.xlf (L126-L132)
ACKs for top commit:
laanwj:
ACK 35d52397e7
Tree-SHA512: cc19e3c09501d240153550d75d7697b5f824cb553f4223beaff66be4d3e6f98d7b5bb14f2d1e1d5ad014eaaa498a7f672e7ff0054ced53ace8c1e6f7e49f6d8a
06c43201a7 cli: use C++17 std::array class template argument deduction (CTAD) (Jon Atack)
edf3167151 addrinfo: raise helpfully on server error or incompatible server version (Jon Atack)
bb85cbc4f7 doc: add cli -addrinfo release note (Jon Atack)
5056a37624 cli: add -addrinfo command (Jon Atack)
db4d2c282a 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 06c43201a7
Tree-SHA512: b668b47718a4ce052aff218789f3da629bca730592c18fcce9a51034d95a0a65f8e6da33dd47443cdd8f60c056c02696db175b0fe09a688e4385a76c1d8b7aeb
de17d245b7 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 712f95d332
It was originally added in 7644567758
<!--
*** 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 de17d245b7.
hebasto:
ACK de17d245b7, I use the same in #21551.
Tree-SHA512: b32428e450cf76297a0e78b5ffb0933ddcfcce010dbd6f5b87d434bf9c437258edb5b817161f6255d5d60523f79f8513a1fe3e92ba62e3fd3786ab042aaae849
e5faec65bd 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 e5faec65bd -- fixed it for me
meshcollider:
ACK e5faec65bd modulo CI
Tree-SHA512: b07d50fd12aa7c239a92aad8ef29f4e88583c3ce701ebedba7c426aac4981c79113adc4670b7d055ab9535a28bdc3f9a30e6ca1b1ed0d7b9a333a3d9c4b40d8a
0b188b751f Clean up context dependent checks in descriptor parsing (Pieter Wuille)
33275a9649 refactor: move uncompressed-permitted logic into ParsePubkey* (Pieter Wuille)
17e006ff8d refactor: split off subscript logic from ToStringHelper (Pieter Wuille)
6ba5dda0c9 Account for key cache indices in subexpressions (Pieter Wuille)
4441c6f3c0 Make DescriptorImpl support multiple subscripts (Pieter Wuille)
a917478db0 refactor: move population of out.scripts from ExpandHelper to MakeScripts (Pieter Wuille)
84f3939ece 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 0b188b751f
Tree-SHA512: cb4e999134aa2bace0e13d4883454c65bcf1369e1c8585d93cc6444ddc245f3def5a628d58af7dab577e9d5a4a75d3bb46f766421fcc8cc5c85c01a11f148b3f
fa8eaee6a8 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871 test: Check that no versionbits are re-used (MarcoFalke)
Pull request description:
ACKs for top commit:
jnewbery:
Code review ACK fa8eaee6a8
ajtowns:
ACK fa8eaee6a8 code review only
Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
bb8d1c6e02 Change ClearDataDirPathCache() to ArgsManager.ClearPathCache(). (Kiminuo)
b4190eff72 Change GetBlocksDir() to ArgsManager.GetBlocksDirPath(). (Kiminuo)
83292e2a70 scripted-diff: Modify unit tests to use the ArgsManager in the BasicTestingSetup class instead of implicitly relying on gArgs. (Kiminuo)
55c68e6f01 scripted-diff: Replace m_args with m_local_args in getarg_tests.cpp (Kiminuo)
511ce3a26b BasicTestingSetup: Add ArgsManager. (Kiminuo)
1cb52ba065 Modify "util_datadir" unit test to not use gArgs. (Kiminuo)
1add318704 Move GetDataDir(fNetSpecific) implementation to ArgsManager. (Kiminuo)
70cdf679f8 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 bb8d1c6e02. Just minor const/naming changes and splitting/scripting commits since last review
MarcoFalke:
review ACK bb8d1c6e02📓
hebasto:
re-ACK bb8d1c6e02, addressed comments, and two commits made scripted-diffs since my [previous](https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-638270583) review.
Tree-SHA512: ba9408c22129d6572beaa103dca0324131766f06d562bb7d6b9e214a0a4d40b0216ce861384562bde24b744003b3fbe6fac239061c8fd798abd3981ebc1b9019
f2f2541ee7 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 f2f2541ee7 :)
jnewbery:
ACK f2f2541ee7
promag:
ACK f2f2541ee7.
Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb
`vcpkg integrate install` must be executed so that msbuild will automatically install external dependencies.
It was removed in 712f95d332
It was originally added in 7644567758
