Commit graph

6019 commits

Author SHA1 Message Date
Ava Chow
bb6de1befb
Merge bitcoin/bitcoin#29034: test: detect OS in functional tests consistently using platform.system()
878d914777 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8 test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3df test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner)

Pull request description:

  There are at least three ways to detect the operating system in Python3:
  - `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
  - `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
  - `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)

  We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release.

  Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via
  ```
  $ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
  ```

  |     OS       | os.name | sys.platform | platform.system()  |
  |--------------|---------|--------------|--------------------|
  | Linux 6.2.0  |  posix  |   linux      |      Linux         |
  | MacOS*       |  posix  |   darwin     |      Darwin        |
  | OpenBSD 7.4  |  posix  |   openbsd7   |      OpenBSD       |
  | Windows*     |  nt     |   win32      |      Windows       |

  \* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed.

ACKs for top commit:
  kevkevinpal:
    ACK [878d914](878d914777)
  achow101:
    ACK 878d914777
  hebasto:
    ACK 878d914777, I have reviewed the code and it looks OK.
  pablomartin4btc:
    tACK 878d914777

Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc
2024-01-11 12:17:01 -05:00
Ava Chow
4e104e2381
Merge bitcoin/bitcoin#28838: test: add assumeutxo wallet test
997b9a73e5 test: add assumeutxo wallet test (Sjors Provoost)

Pull request description:

  Extracted from #28616, this adds a (very) basic wallet test for assume utxo. It checks some circumstances where a backup can and can't be loaded.

ACKs for top commit:
  maflcko:
    lgtm ACK 997b9a73e5
  achow101:
    ACK 997b9a73e5
  theStack:
    Code-review ACK 997b9a73e5

Tree-SHA512: 69474e56c6a46bb4f30fc54f8e5844766ac2a5f8226bb0b168d11ae1e3d4eae58570c1f1b4cc2b2f6f51b5d0e055bbe2bbd11684265215e01d4eb81ab4b7b0bb
2024-01-11 11:30:18 -05:00
kevkevin
0eebd6fe7d
test: Assert that a new tx with a delta of 0 is never added 2024-01-11 08:16:42 -06:00
kevkevin
cfdbcd19b3
rpc: exposing modified_fee in getprioritisedtransactions
Instead of having modified_fee be hidden we are now exposing it to avoid
having useless code
2024-01-11 08:16:22 -06:00
Ava Chow
fcacbab487
Merge bitcoin/bitcoin#29204: test: wallet migration, add coverage for tx extra data
016cc807f7 test: wallet migration, add coverage for tx extra data (furszy)

Pull request description:

  Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.

  Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
  as well as the extra tx comments.

ACKs for top commit:
  jamesob:
    Nice, ACK 016cc807f7
  achow101:
    ACK 016cc807f7
  pablomartin4btc:
    ACK 016cc807f7
  BrandonOdiwuor:
    lgtm ACK 016cc807f7

Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
2024-01-10 14:35:22 -05:00
Ava Chow
7ff8e6b240
Merge bitcoin/bitcoin#28318: logging: Simplify API for level based logging
e60fc7d5d3 logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a056 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e329 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012 logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874 logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615 logging: refactor: pull prefix code out (Anthony Towns)

Pull request description:

  Replace `LogPrint*` functions with severity based logging functions:

   * `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
   * `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
   * `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)

  Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.

  Adds docs to developer-notes about when to use which level.

  Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.

  Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.

ACKs for top commit:
  maflcko:
    re-ACK e60fc7d5d3 🌚
  achow101:
    ACK e60fc7d5d3
  stickies-v:
    ACK e60fc7d5d3
  jamesob:
    ACK e60fc7d5d3 ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))

Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
2024-01-10 14:11:32 -05:00
Sebastian Falbesoner
931575418e test: assumeutxo: spend coin from snapshot chainstate after loading
Check that an UTXO that is only available in the snapshot chainstate
is also visible to the mempool by submitting a spending transaction.
2024-01-10 01:18:27 +01:00
furszy
016cc807f7
test: wallet migration, add coverage for tx extra data
Verifying that the 'replaced_by_txid' and 'replaces_txid'
tx data is preserved after migration, as well as the
extra tx comments.
2024-01-08 12:04:31 -03:00
fanquake
c2d04f1319
Merge bitcoin/bitcoin#28610: wallet: Migrate entire address book entries to watchonly and solvables too
406b71abcb wallet: Migrate entire address book entries (Andrew Chow)

Pull request description:

  Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.

ACKs for top commit:
  ryanofsky:
    Code review ACK 406b71abcb. Just suggested change since last review
  furszy:
    Code review ACK 406b71ab

Tree-SHA512: 13de42b16a1d8524fe0555764744139566b2e7d29741ceffc1158a905dd537136b762330568b3b5cac28cbee1bfd363a20de97d0a6c5296738cb3aa99133945b
2024-01-08 14:44:47 +00:00
Fabian Jahr
6044628543
crypto, hash: replace custom rotl32 with std::rotl 2024-01-05 17:12:38 +01:00
fanquake
143ace65db
Merge bitcoin/bitcoin#28890: rpc: Remove deprecated -rpcserialversion
fa46cc22bc Remove deprecated -rpcserialversion (MarcoFalke)

Pull request description:

  The flag is problematic for many reasons:

  * It is deprecated
  * It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
  * It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see https://github.com/bitcoin/bitcoin/issues/28730#issuecomment-1780940868
  * It makes performance improvements harder to implement: https://github.com/bitcoin/bitcoin/pull/17529#issuecomment-556082818

  Fix all issues by removing it.

  If there is a use-case, likely a per-RPC flag can be added, if needed.

ACKs for top commit:
  ajtowns:
    crACK fa46cc22bc
  TheCharlatan:
    lgtm ACK fa46cc22bc

Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
2024-01-05 10:42:10 +00:00
Anthony Towns
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace
These provide simple and clear ways to write the most common logging
operations:

    LogInfo("msg");
    LogDebug(BCLog::LogFlags::NET, "msg");

    LogError("msg");
    LogWarning("msg");
    LogTrace(BCLog::LogFlags::NET, "msg");

For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
2023-12-20 15:59:48 +10:00
ismaelsadeeq
8dec9c560b wallet, mempool: propagete checkChainLimits error message to wallet
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.

Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
2023-12-17 21:13:44 +01:00
Ava Chow
3695ecbf68
Merge bitcoin/bitcoin#29088: tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG
7b45744df3 tests: ensure functional tests set permitbaremultisig=1 when needed (Anthony Towns)
7dfabdcf86 tests: test both settings for permitbaremultisig in p2sh tests (Anthony Towns)

Pull request description:

  Update unit and functional tests so that they continue to work if the default for `-permitbaremultisig` is changed.

ACKs for top commit:
  maflcko:
    lgtm ACK 7b45744df3
  instagibbs:
    crACK 7b45744df3
  ajtowns:
    > crACK [7b45744](7b45744df3)
  achow101:
    ACK 7b45744df3
  glozow:
    ACK 7b45744df3, changed default locally and all tests passed

Tree-SHA512: f89f9e2bb11f07662cfd57390196df9e531064e1bd662e1db7dcfc97694394ae5e8014e9d209b9405aa09195bf46fc331b7fba10378065cdb270cbd0669ae904
2023-12-15 16:22:54 -05:00
Anthony Towns
7b45744df3 tests: ensure functional tests set permitbaremultisig=1 when needed
The mempool_dust and mempool_sigoplimits functional tests both use bare
multisig txs, so ensure they're allowed by policy.
2023-12-15 18:37:29 +10:00
Ava Chow
08e6aaabef
Merge bitcoin/bitcoin#28920: wallet: birth time update during tx scanning
1ce45baed7 rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c66444d0 test: coverage for wallet birth time interaction with -reindex (furszy)
6f497377aa wallet: fix legacy spkm default birth time (furszy)
75fbf444c1 wallet: birth time update during tx scanning (furszy)
b4306e3c8d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)

Pull request description:

  Fixing #28897.

  As the user may have imported a descriptor with a timestamp newer
  than the actual birth time of the first key (by setting 'timestamp=now'),
  the wallet needs to update the birth time when it detects a transaction
  older than the oldest descriptor timestamp.

  Testing Notes:
  Can cherry-pick the test commit on top of master. It will fail there.

ACKs for top commit:
  Sjors:
    re-utACK 1ce45baed7
  achow101:
    ACK 1ce45baed7

Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9
2023-12-14 16:27:40 -05:00
Ava Chow
9860471708
Merge bitcoin/bitcoin#29070: test: add TestNode wait_until helper
bf0f7dbec6 test: add TestNode wait_until helper (Nikodemas Tuckus)

Pull request description:

  Add `wait_until` method that wraps the `wait_until_helper_internal` call.

  Closes https://github.com/bitcoin/bitcoin/issues/29029.

ACKs for top commit:
  maflcko:
    lgtm ACK bf0f7dbec6
  mohamedawnallah:
    LGTM! Code Review ACK bf0f7dbec6
  achow101:
    ACK bf0f7dbec6
  BrandonOdiwuor:
    Code review ACK bf0f7dbec6

Tree-SHA512: 05aab589c814f51a14e1483eb57c10b88385714e3eb2d0973c0ee2877f2b963a76837f34215fe2e6bd1c8d735f5af7dd2098331e1eda28587f39e513bc6e1a6a
2023-12-14 14:19:16 -05:00
Ava Chow
4d7b787ad6
Merge bitcoin/bitcoin#29022: Make bitcoin-tx replaceable value optional
98afe78661 doc: Update bitcoin-tx replaceable documentation (Kashif Smith)
94feaf2b66 tests: Add unit tests for bitcoin-tx replaceable command (Kashif Smith)
c2b836b119 bitcoin-tx: Make replaceable value optional (Kashif Smith)

Pull request description:

  This fixes #28638. The issue was originally raised by dooglus, who also suggested the patch found in this code. Additionally, test coverage has been added and documentation has been updated.

ACKs for top commit:
  achow101:
    ACK 98afe78661
  pinheadmz:
    ACK 98afe78661
  hernanmarino:
    Tested ACK 98afe78661
  instagibbs:
    untested ACK 98afe78661

Tree-SHA512: ea1384aba7b0014c8cbeb7280d66b1e617d406fb02471dff33873057132b80518c94c7caa4b0426c26d17ce8aa393107de319dde781ace8df72f0314c8c75159
2023-12-14 13:54:00 -05:00
Nikodemas Tuckus
bf0f7dbec6 test: add TestNode wait_until helper 2023-12-13 11:24:03 +01:00
MarcoFalke
fa0534d7e4
test: Actually fail when a python unit test fails 2023-12-12 21:18:08 +01:00
MarcoFalke
fa46cc22bc
Remove deprecated -rpcserialversion 2023-12-11 18:22:13 +01:00
fanquake
36fabb01b1
Merge bitcoin/bitcoin#29041: test: fix intermittent error in rpc_net.py (#29030)
ea00f982d2 test: fix intermittent error in rpc_net.py (#29030) (Sebastian Falbesoner)

Pull request description:

  Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point.

  Solve this by using the recently introduced `wait_for_new_peer` helper (see #29006,  commit 00e0658e77), which is more robust.

  Fixes #29030.

ACKs for top commit:
  maflcko:
    lgtm ACK ea00f982d2

Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
2023-12-11 11:41:46 +01:00
Sebastian Falbesoner
ea00f982d2 test: fix intermittent error in rpc_net.py (#29030)
Asserting for the debug log message "Added connection peer=" is
insufficient for ensuring that this new connection will show up in a
following getpeerinfo() call, as the debug message is written in the
CNode ctor, which means it hasn't necessarily been added to
CConnman.m_nodes at this point.

Solve this by using the recently introduced `wait_for_new_peer`
helper, which is more robust.

Fixes #29030.
2023-12-09 13:26:18 +01:00
Kashif Smith
94feaf2b66 tests: Add unit tests for bitcoin-tx replaceable command 2023-12-08 20:27:13 -05:00
Sebastian Falbesoner
fd0bde2793 test: fix addnode functional test failure on OpenBSD
This is the functional test counterpart of PR #28891 /
commit 007d6f0e85.
2023-12-08 18:27:15 +01:00
Sebastian Falbesoner
878d914777 doc: test: mention OS detection preferences in style guideline 2023-12-08 18:16:27 +01:00
Sebastian Falbesoner
4c65ac96f8 test: detect OS consistently using platform.system() 2023-12-08 18:16:24 +01:00
Sebastian Falbesoner
37324ae3df test: use skip_if_platform_not_linux helper where possible
Rather than re-implementing these checks, we can use this test
framework's helper (introduced in commit
c934087b62, PR #24358) called in a test's
`skip_test_if_missing_module` method instead.
2023-12-08 18:15:34 +01:00
fanquake
03042fb6bb
Merge bitcoin/bitcoin#29006: test: fix v2 transport intermittent test failure (#29002)
00e0658e77 test: fix v2 transport intermittent test failure (#29002) (Sebastian Falbesoner)

Pull request description:

  This PR improves the following fragile construct for detection of a new connection to the node under test in `p2p_v2_transport.py`:
  6d5790956f/test/functional/p2p_v2_transport.py (L154-L156)
  Only relying on the number of peers for that suffers from race conditions, as unrelated previous peers could disconnect at anytime in-between. In the test run in #29002, the following happens:

  - `getpeerinfo()` is called the first time -> assigned to `num_peers`
  - **previous peer disconnects**, the node's peer count is now `num_peers - 1` (in most test runs, this happens before the first getpeerinfo call)
  - new peer connects, the node's peer count is now `num_peers`
  - the condition that the node's peer count is `num_peers + 1` is never true, test fails

  Use the more robust approach of watching for an increased highest peer id instead (again using the `getpeerinfo` RPC call), with a newly introduced context manager method `TestNode.wait_for_new_peer()`. Note that for the opposite case of a disconnect, no new method is introduced; this is currently used only once in the test and is also simpler.

  Still happy to take suggestions for alternative solutions.

  Fixes #29002.

ACKs for top commit:
  kevkevinpal:
    Concept ACK [00e0658](00e0658e77)
  maflcko:
    Ok, lgtm ACK 00e0658e77
  stratospher:
    ACK 00e0658.

Tree-SHA512: 0118b87f54ea5e6e080ff44f29d6af6674c757a588534b3add040da435f4359e71bf85bc0a5eb7170f99cc9956e1a03c35cce653d642d31eed41bbed1f94f44f
2023-12-08 11:33:27 +00:00
fanquake
1f352cf2fd
Merge bitcoin/bitcoin#28485: test: Extends MEMPOOL msg functional test
97c0dfa894 test: Extends MEMPOOL msg functional test (Sergi Delgado Segura)

Pull request description:

  Currently, p2p_filter.py::test_msg_mempool is not testing much. This extends the tests so the interaction between sending `MEMPOOL` messages with a filter that does not include all transactions in the mempool reacts, plus how it interacts with `INV` messages, especially after the changes introduced by #27675

ACKs for top commit:
  instagibbs:
    ACK 97c0dfa894
  theStack:
    re-ACK 97c0dfa894

Tree-SHA512: 746fdc867630f40509e6341f484a238dd855ae6d1be5eca121974491e4ca272dee88af4b90dda55ea9a5a19cbff198fa91ffa0c5bf1ddf0232b2c1b215b05b9a
2023-12-08 11:28:59 +00:00
Sergi Delgado Segura
97c0dfa894 test: Extends MEMPOOL msg functional test
Currently, p2p_filter.py::test_msg_mempool is not testing much.
This extends the tests so the interaction between sending MEMPOOL messages with
a filter that does not include all transactions in the mempool reacts, plus how
it interacts with INV messages
2023-12-06 13:21:58 -05:00
Andrew Chow
c46cc8d3c1
Merge bitcoin/bitcoin#27581: net: Continuous ASMap health check
3ea54e5db7 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37ae fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)

Pull request description:

  There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.

  This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.

ACKs for top commit:
  achow101:
    ACK 3ea54e5db7
  mzumsande:
    ACK 3ea54e5db7
  brunoerg:
    crACK 3ea54e5db7

Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
2023-12-06 11:22:42 -05:00
Sebastian Falbesoner
00e0658e77 test: fix v2 transport intermittent test failure (#29002)
Only relying on the number of peers for detecting a new connection
suffers from race conditions, as unrelated previous peers could
disconnect at anytime in-between. Use the more robust approach of
watching for an increased highest peer id instead (again using the
`getpeerinfo` RPC call), with a newly introduced context manager
method `TestNode.wait_for_new_peer()`.

Fixes #29009.
2023-12-06 00:28:28 +01:00
Martin Zumsande
9075a44646 test: add regression test for the getrawtransaction segfault
This fails on master without the previous commit.
2023-12-05 17:12:56 -05:00
furszy
1ce45baed7
rpc: getwalletinfo, return wallet 'birthtime'
And add coverage for it
2023-12-05 18:55:35 -03:00
furszy
83c66444d0
test: coverage for wallet birth time interaction with -reindex
Verifying the wallet updates the birth time accordingly when it
detects a transaction with a time older than the oldest descriptor
timestamp.
This could happen when the user blindly imports a descriptor with
'timestamp=now'.
2023-12-05 18:55:35 -03:00
Andrew Chow
406b71abcb wallet: Migrate entire address book entries 2023-12-05 13:35:55 -05:00
willcl-ark
8f6ab31863
init: don't delete PID file if it was not generated
Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.

Fix this to only delete the PID file if we created it.
2023-12-04 12:54:20 +00:00
Fabian Jahr
3ea54e5db7
net: Add continuous ASMap health check logging 2023-12-02 22:03:08 +01:00
Andrew Chow
18bed148af
Merge bitcoin/bitcoin#28784: rpc: keep .cookie file if it was not generated
7cb9367157 rpc: keep .cookie if it was not generated (Roman Zeyde)

Pull request description:

  Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).

ACKs for top commit:
  willcl-ark:
    re-ACK 7cb9367157
  achow101:
    ACK 7cb9367157
  kristapsk:
    re-ACK 7cb9367157
  stickies-v:
    ACK 7cb9367157

Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b
2023-12-01 12:24:29 -05:00
Andrew Chow
6b3927f79a
Merge bitcoin/bitcoin#28848: bugfix, Change up submitpackage results to return results for all transactions
f23ba24aa0 test_submitpackage: only make a chain of 3 txns (Greg Sanders)
e67a345162 doc: submitpackage vsize results are sigops-adjusted (Greg Sanders)
b67db52c39 RPC submitpackage: change return format to allow partial errors (Greg Sanders)

Pull request description:

  This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.

  Added a top level `package_msg` for quick eye-balling of general success of the package.

  This PR also fixes a couple bugs:

  1) Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes `PKG_TX` failure.
  2) "other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.

ACKs for top commit:
  Sjors:
    Light re-utACK f23ba24aa0
  achow101:
    ACK f23ba24aa0
  glozow:
    utACK f23ba24aa0, thanks for taking the suggestions

Tree-SHA512: ebfd716a4fed9e8c2dea3d2181ba6a6171b06718d29ac2324c67b7a30b374d199f7e1739f91ab5d036be172d0479de9bc89c32263ee62143c0338b9b622d0cca
2023-12-01 12:17:15 -05:00
Greg Sanders
f23ba24aa0 test_submitpackage: only make a chain of 3 txns 2023-11-29 12:56:26 -05:00
Greg Sanders
b67db52c39 RPC submitpackage: change return format to allow partial errors
Behavior prior to this commit allows some transactions to
enter into the local mempool but not be reported to the user
when encountering a PackageValidationResult::PCKG_TX result.

This is further compounded with the fact that any transactions
submitted to the mempool during this call would also not be
relayed to peers, resulting in unexpected behavior.

Fix this by, if encountering a package error, reporting all
wtxids, along with a new error field, and broadcasting every
transaction that was found in the mempool after submission.

Note that this also changes fees and vsize to optional,
which should also remove an issue with other-wtxid cases.
2023-11-29 12:56:26 -05:00
MarcoFalke
fa9dc92c53
test: Add missing CBlockPolicyEstimator::processBlockTx suppression 2023-11-29 10:51:04 +01:00
Andrew Chow
75462b39d2
Merge bitcoin/bitcoin#28554: bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC
9ac114e5cd Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)

Pull request description:

  When writing some scripts that iterated over many blocks to generate hashrate estimates I realized that my script was going out of range of the current chain tip height but was not encountering any errors.

  I believe that passing an invalid block height to this function but receiving the hashrate estimate for the chain tip instead should be considered unexpected behavior.

ACKs for top commit:
  Sjors:
    re-utACK 9ac114e5cd
  kevkevinpal:
    reACK [9ac114e](9ac114e5cd)
  achow101:
    ACK 9ac114e5cd

Tree-SHA512: eefb465c2dd654fc48267f444e1809597ec5363cdd131ea9ec812458fed1e4bffbbbb0617d74687c9f7bb16274b598d8292f5eeb7953421e5d2a8dc2cc081f2b
2023-11-28 16:26:04 -05:00
Andrew Chow
30a0557829
Merge bitcoin/bitcoin#28805: test: Make existing functional tests compatible with --v2transport
35fb9930ad test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c2695 test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a9001751 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)

Pull request description:

  This makes the functional test suite compatible with BIP324, so that
  `python3 test_runner.py --v2transport`
  should succeed (currently, 12 tests fail for me on master).
  Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164

  Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
  Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.

ACKs for top commit:
  sipa:
    utACK 35fb9930ad. Thanks for taking care of this.
  achow101:
    ACK 35fb9930ad
  theStack:
    ACK 35fb9930ad
  stratospher:
    ACK 35fb993.

Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3
2023-11-28 14:16:35 -05:00
MarcoFalke
fa20f8919c
test: Add gettransaction test for "coin-join" tx 2023-11-23 14:11:01 +01:00
fanquake
172cd92620
Merge bitcoin/bitcoin#28862: lint: Report all lint errors instead of early exit
fa01f884d3 ci: Add missing COPY for ./test/lint/test_runner (MarcoFalke)
faff3e3b46 lint: Report all lint errors instead of early exit (MarcoFalke)

Pull request description:

  `all-lint.py` currently collects all failures. However, the `06_script.sh` does not, since July this year (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806).

  Fix this by printing all failures before exiting.

  Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.

ACKs for top commit:
  kevkevinpal:
    ACK [fa01f88](fa01f884d3)
  TheCharlatan:
    lgtm ACK fa01f884d3

Tree-SHA512: c0f3110f2907d87e29c755e3b77a67dfae1f8a25833fe6ef8f2f2c58cfecf1aa46f1a20881576b62252b04930140a9e416c78b4edba0780d3c4fa7aaebabba81
2023-11-22 17:00:45 +00:00
kevkevin
3a118e19e1
test: Directly constructing 2 entry map for getprioritisedtransactions
Directly constructing the map in the assertion instead of indexing by txid
so that we wouldn't miss new entries in future regressions.
2023-11-21 18:02:08 -06:00
fanquake
98b0acda0f
Merge bitcoin/bitcoin#28725: test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)
a478c817b2 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)

Pull request description:

  With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see  https://peps.python.org/pep-0585/#implementation for the full list).

  This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:

  - typing.Dict -> dict
  - typing.List -> list
  - typing.Set  -> set
  - typing.Tuple -> tuple

  For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
  ```
  $ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
  ```
  and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).

ACKs for top commit:
  stickies-v:
    ACK a478c817b2
  fanquake:
    ACK a478c817b2

Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
2023-11-17 11:19:17 +00:00
stickies-v
a478c817b2 test: replace Callable/Iterable with their collections.abc alternative (PEP 585) 2023-11-16 19:12:14 +01:00
fanquake
22025d06e5
Merge bitcoin/bitcoin#28605: Fix typos
43de4d3630 doc: fix typos (Sjors Provoost)

Pull request description:

  This PR fixes typos found by lint-spelling.py using codespell 2.2.6.

  Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.

ACKs for top commit:
  pablomartin4btc:
    re ACK 43de4d3630

Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
2023-11-16 10:35:49 +00:00
fanquake
fd30e9688e
test: migrate to some per-symbol ubsan suppressions
Tested on aarch64 using the ASAN CI job. Currently unable to test on
x86_64 due to AppArmor & podman issues.
2023-11-14 15:52:55 +00:00
MarcoFalke
faff3e3b46
lint: Report all lint errors instead of early exit 2023-11-13 18:08:10 +01:00
Andrew Chow
d232e36abd
Merge bitcoin/bitcoin#28207: mempool: Persist with XOR
fa6b053b5c mempool: persist with XOR (MarcoFalke)

Pull request description:

  Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

  While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.

  Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat file when writing or reading it.

  Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.

ACKs for top commit:
  achow101:
    re-ACK fa6b053b5c
  glozow:
    reACK fa6b053b5c
  ismaelsadeeq:
    ACK fa6b053b5c

Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
2023-11-13 11:28:15 -05:00
fanquake
6342348072
Merge bitcoin/bitcoin#28076: util: Replace std::filesystem with util/fs.h
bbbbdb0cd5 ci: Add filesystem lint check (MarcoFalke)
fada2f9110 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)

Pull request description:

  Using `std::filesystem` is problematic:

  * There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
  * Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.

  Fix all issues by removing use of it and adding a linter to avoid using it again in the future.

ACKs for top commit:
  TheCharlatan:
    ACK  bbbbdb0cd5
  fanquake:
    ACK bbbbdb0cd5 🦀

Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
2023-11-13 14:10:54 +00:00
Sjors Provoost
997b9a73e5
test: add assumeutxo wallet test
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
2023-11-13 15:19:12 +04:00
fanquake
e11b7587a1
Merge bitcoin/bitcoin#28831: test: Avoid intermittent failures in feature_init
44445ae8f1 test: Avoid intermittent failures in feature_init (MarcoFalke)

Pull request description:

  The code not only modifies block dat files, but also leveldb files, which may be of smaller size. Such corruption may not force leveldb to abort, according to the intermittent test failures.

  Fix the intermittent test failures by reverting 5ab6419f38 .

ACKs for top commit:
  kevkevinpal:
    lgtm ACK [44445ae](44445ae8f1)
  fjahr:
    ACK 44445ae8f1
  theStack:
    ACK 44445ae8f1

Tree-SHA512: 8084e4aeb8a976c1706a1898d7854c55d0c4ec7b5a08f65f97ffc173c935f9b0e0c1caef7be1538a458e4c018f7bd1948173349ec76ca48bc4013a63f284bb0f
2023-11-13 10:12:34 +00:00
Kashif Smith
22e38080ea test: fix node index bug when comparing peerinfo 2023-11-10 16:25:18 -05:00
fanquake
1fdd832842
Merge bitcoin/bitcoin#28835: test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698
7de7685372 test, assumeutxo: Use assert_debug_log for error details (pablomartin4btc)

Pull request description:

  This is a follow-up on the invalid hash dump fix #28698, [suggested](https://github.com/bitcoin/bitcoin/pull/28698#pullrequestreview-1698178157) by theStack and agreed by Sjors and ryanofsky.

ACKs for top commit:
  Sjors:
    ACK 7de7685372
  maflcko:
    lgtm ACK 7de7685372

Tree-SHA512: 036b3cef3084e3ead8923e8dcabe4fa7ebe97fb514d223aa38bc38df10337e3fe3113e42322178b58fb03fcd4511af4b5b56bceecbb7ded5b9758842c70db3f2
2023-11-10 09:55:56 +00:00
pablomartin4btc
7de7685372 test, assumeutxo: Use assert_debug_log for error details
This is a follow-up on the invalid hash dump fix PR #28698.

https://github.com/bitcoin/bitcoin/pull/28698#pullrequestreview-1698178157
2023-11-09 18:54:27 -03:00
MarcoFalke
fa6b053b5c
mempool: persist with XOR 2023-11-09 19:44:50 +01:00
MarcoFalke
44445ae8f1
test: Avoid intermittent failures in feature_init 2023-11-09 14:30:02 +01:00
fanquake
c2da8c583f
Merge bitcoin/bitcoin#28822: test: Add missing wait for version to be sent in add_outbound_p2p_connection
faa2ad88bc test: Add missing wait for version to be sent in add_outbound_p2p_connection (MarcoFalke)

Pull request description:

  Can be tested with:

  ```diff
  diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
  index b1ed97b794..eb4f72c6b6 100755
  --- a/test/functional/test_framework/p2p.py
  +++ b/test/functional/test_framework/p2p.py
  @@ -205,6 +205,7 @@ class P2PConnection(asyncio.Protocol):
           assert not self._transport
           logger.debug("Connected & Listening: %s:%d" % (self.dstaddr, self.dstport))
           self._transport = transport
  +        import time;time.sleep(.1);
           if self.on_connection_send_msg:
               self.send_message(self.on_connection_send_msg)
               self.on_connection_send_msg = None  # Never used again
  ```

  Found and reported by mzumsande in https://github.com/bitcoin/bitcoin/pull/28782#pullrequestreview-1718560252

ACKs for top commit:
  mzumsande:
    ACK faa2ad88bc

Tree-SHA512: 863f06125dec40cccaa852d0d8ca2e2b9c0b74610205e9fd6c9c279bdf36801ff475e3d873fd1b18172eb8220e17b2caff60069ce63512e569934a43f27d03fd
2023-11-09 10:26:56 +00:00
Martin Zumsande
35fb9930ad test: enable v2 transport for p2p_timeouts.py
by skipping the part where we send a non-version message
before the version - this message would be interpreted as
part of the v2 handshake.
2023-11-08 17:34:50 -05:00
Sebastian Falbesoner
2c1669c37a test: enable v2 transport for rpc_net.py
- "transport_protocol_type" of inbound peer before version handshake
  is "detecting" on p2p v2 nodes (as opposed to "v1" for p2p v1)
- size of a ping/pong message is 29 bytes (as opposed to 32 for p2p v1)
- for the sendmsgtopeer RPC sub-test, enforce p2p v1 connection to
  have a peer id of zero
2023-11-08 17:34:50 -05:00
Sebastian Falbesoner
cc961c2695 test: enable v2 transport for p2p_node_network_limited.py 2023-11-08 17:34:50 -05:00
Martin Zumsande
3598a1b5c9 test: enable --v2transport in combination with --usecli
By renaming the "command" send_cli arg. The old name was unsuitable
because the "addnode" RPC has its own "command" arg, leading to
ambiguity when included in kwargs.
Can be tested with
"python3 wallet_multiwallet.py --usecli --v2transport"
which fails on master because of this (python throws a TypeError).
2023-11-08 17:34:50 -05:00
Martin Zumsande
68a9001751 test: persist -v2transport over restarts and respect -v2transport=0
Before, a global -v2transport provided to the test would be dropped
when restarting the node within a test and specifying any extra_args.

Fix this by adding "v2transport=1" to args (not extra_args) based
on the global parameter, and deciding for each (re)start of the node
based on this default and test-specific extra_args
(which take precedence over args) whether v2 should be used.
2023-11-08 17:30:20 -05:00
Andrew Chow
19d1ba1b41
Merge bitcoin/bitcoin#28787: init: completely remove -zapwallettxes (remaining hidden option)
5039c346ca init: completely remove `-zapwallettxes` (remaining hidden option) (Sebastian Falbesoner)

Pull request description:

  The `-zapwallettxes` functionality has been removed in v0.21.0 (see commit 3340dbadd3 / PR #19671), with the parameter being kept as hidden option, to inform users via an exit error that `abandontransaction` should be used instead.

  As any guides that still suggest to use `-zapwallettxes` would refer to a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x), it is highly unlikely that the error caused by the option is still relevant for any user, hence it seems fine to remove it now.

ACKs for top commit:
  achow101:
    ACK 5039c346ca
  BrandonOdiwuor:
    ACK 5039c346ca
  fanquake:
    ACK 5039c346ca

Tree-SHA512: e3ccc6918e0f8fa68dbd1a7ec4999cc2a44e28038711919fcddaf0727648c73a9ba0fb77674317147592a113fad20755d4e727f48176bc17b048fbdebad2d6c9
2023-11-08 10:53:42 -05:00
glozow
9ad19fc7c7
Merge bitcoin/bitcoin#28155: net: improves addnode / m_added_nodes logic
0420f99f42 Create net_peer_connection unit tests (Jon Atack)
4b834f6499 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443b net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)

Pull request description:

  ## Rationale

  Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.

  ### Connecting to the same node more than once

  In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:

  1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
  2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP

  For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.

  An example to test this would be calling:

  ```
  bitcoin-cli addnode "127.0.0.1:port" add
  bitcoin-cli addnode "localhost:port" add
  ```

  And check how it allows us to perform both connections some times, and some times it fails.

  The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:

  ```
  bitcoin-cli addnode "127.0.0.1:port" add
  bitcoin-cli addnode "localhost:port" onetry
  ```

  ### Adding the same peer with two different, yet equivalent, addresses

  The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.

  Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.

  This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.

  ### Parametrize `CConnman::GetAddedNodeInfo`
  Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`

ACKs for top commit:
  jonatack:
    re-ACK 0420f99f42
  kashifs:
    > > tACK [0420f9](0420f99f42)
  sr-gi:
    > > > tACK [0420f9](0420f99f42)
  mzumsande:
    Tested ACK 0420f99f42

Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
2023-11-08 11:31:36 +00:00
MarcoFalke
faa2ad88bc
test: Add missing wait for version to be sent in add_outbound_p2p_connection 2023-11-08 11:36:24 +01:00
fanquake
059f131314
Merge bitcoin/bitcoin#28820: tests: Increase wallet_miniscript.py rpc timeout to 90 seconds
6559e4d27a tests: Increase wallet_miniscript.py rpc timeout to 90 seconds (Andrew Chow)

Pull request description:

  The signing test for the large miniscript can sometimes take longer than the 30 second timeout, depending on the load on my system. Increasing it to 90 seconds seems to be good enough.

ACKs for top commit:
  kevkevinpal:
    but increasing seems fine ACK [6559e4d](6559e4d27a)
  maflcko:
    lgtm ACK 6559e4d27a

Tree-SHA512: 1b7bf94c77f85a0deddb1384aacbeb934205d0a630fecc8e75a4a98d1946d77d9bca36692fb6c1ab8e9276392f617281aafc4c685c248a8d3b0c77f896cda624
2023-11-08 09:56:49 +00:00
fanquake
1162d046ec
Merge bitcoin/bitcoin#28782: test: Add missing sync on send_version in peer_connect
fa02598469 test: Add missing sync on send_version in peer_connect (MarcoFalke)

Pull request description:

  Without the sync, the logic will be racy. For example, `p2p_sendtxrcncl.py` is failing locally (and on CI occasionally), because non-version messages will be sent before the version message:

  ```py
          self.log.info('SENDTXRCNCL with version=0 triggers a disconnect')
          sendtxrcncl_low_version = create_sendtxrcncl_msg()
          sendtxrcncl_low_version.version = 0
          peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
          with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]):
              peer.send_message(sendtxrcncl_low_version)
              peer.wait_for_disconnect()
  ```

  ```
   test  2023-11-02T08:15:19.620000Z TestFramework (INFO): SENDTXRCNCL with version=0 triggers a disconnect
   test  2023-11-02T08:15:19.621000Z TestFramework.p2p (DEBUG): Connecting to Bitcoin Node: 127.0.0.1:11312
   test  2023-11-02T08:15:19.624000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:11312
   test  2023-11-02T08:15:19.798000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_sendtxrcncl(version=0, salt=2)
   test  2023-11-02T08:15:19.799000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_version(nVersion=70016 nServices=9 nTime=Thu Nov  2 08:15:19 2023 addrTo=CAddress(nServices=1 net=IPv4 addr=127.0.0.1 port=11312) addrFrom=CAddress(nServices=1 net=IPv4 addr=0.0.0.0 port=0) nNonce=0x369AC031CDA96022 strSubVer=/python-p2p-tester:0.0.3/ nStartingHeight=-1 relay=1)
   node0 2023-11-02T08:15:19.804409Z [net] [net.cpp:3676] [CNode] [net] Added connection peer=0
   node0 2023-11-02T08:15:19.805256Z [net] [net.cpp:1825] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:55964 accepted
   node0 2023-11-02T08:15:19.809861Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: sendtxrcncl (12 bytes) peer=0
   node0 2023-11-02T08:15:19.810297Z [msghand] [net_processing.cpp:3582] [ProcessMessage] [net] non-version message before version handshake. Message "sendtxrcncl" from peer=0
   node0 2023-11-02T08:15:19.810928Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: version (111 bytes) peer=0
  ...
   test  2023-11-02T09:35:20.166000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
                                             def test_function():
                                                 if check_connected:
                                                     assert self.is_connected
                                                 return test_function_in()
                                     '''
   test  2023-11-02T09:35:20.187000Z TestFramework (ERROR): Assertion failed
                                     Traceback (most recent call last):
                                       File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
                                         self.run_test()
                                       File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/p2p_sendtxrcncl.py", line 188, in run_test
                                         peer.wait_for_disconnect()
                                       File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 478, in wait_for_disconnect
                                         self.wait_until(test_function, timeout=timeout, check_connected=False)
                                       File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 470, in wait_until
                                         wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
                                       File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/util.py", line 275, in wait_until_helper_internal
                                         raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
                                     AssertionError: Predicate ''''
                                             def test_function():
                                                 if check_connected:
                                                     assert self.is_connected
                                                 return test_function_in()
                                     ''' not true after 4800.0 seconds

ACKs for top commit:
  mzumsande:
    ACK fa02598469

Tree-SHA512: 78871f603d387e2df8c0acbdfa95441fa186f80e94593021bb219bbf1bc9dc7efc4e266bd254b5cc41114c38227ff3b7f6172335d9bb828427f0a2acffde752d
2023-11-08 09:55:53 +00:00
fanquake
b5d8f001a8
Merge bitcoin/bitcoin#28814: test: symbolizer improvements
49d953281d fuzz: explicitly specify llvm-symbolizer path in runner (fanquake)

Pull request description:

  It's not completely clear to me why this needs to be explicitly specified in some environments, and not in others, while at the same time that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues outlined in https://github.com/bitcoin/bitcoin/pull/28147.

  Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize script, but not in in the ubsan_symbolize script, or from in compiler-rt.

  Alternative to #28804.

ACKs for top commit:
  maflcko:
    lgtm ACK 49d953281d

Tree-SHA512: c3d5bf1c3629793b342c70754a419b3c7a3cd39f800b9aa69ce3395cc2bf83b4d46f2b329974337b94b99573cd0b8600d3f147ed5c21387bf3812316570d1ee3
2023-11-08 09:46:51 +00:00
Andrew Chow
6559e4d27a tests: Increase wallet_miniscript.py rpc timeout to 90 seconds
The signing test for the large miniscript can sometimes take longer than
the 30 second timeout, depending on the load on my system. Increasing it
to 90 seconds seems to be good enough.
2023-11-07 18:32:28 -05:00
Andrew Chow
962ea5c525
Merge bitcoin/bitcoin#28374: test: python cryptography required for BIP 324 functional tests
c534c08710 [test/crypto] Add FSChaCha20Poly1305 AEAD python implementation (stratospher)
c2a458f1c2 [test/crypto] Add FSChaCha20 python implementation (stratospher)
c4ea5f6288 [test/crypto] Add RFC 8439's ChaCha20Poly1305 AEAD (stratospher)
9fc6e0355e [test/crypto] Add Poly1305 python implementation (stratospher)
fec2ca6c9a [test/crypto] Use chacha20_block function in `data_to_num3072` (stratospher)
0cde60da3a [test/crypto] Add ChaCha20 python implementation (stratospher)
69d3f50ab6 [test/crypto] Add HMAC-based Key Derivation Function (HKDF) (stratospher)
08a4a56cbc [test] Move test framework crypto functions to crypto/ (stratospher)

Pull request description:

  split off from #24748 to keep commits related to cryptography and functional test framework changes separate.

  This PR adds python implementation and unit tests for HKDF, ChaCha20, Poly1305, ChaCha20Poly1305 AEAD, FSChaCha20 and FSChaCha20Poly1305 AEAD.

  They're based on cc177ab7bc/bip-0324/reference.py for easy review.

ACKs for top commit:
  sipa:
    utACK c534c08710
  achow101:
    ACK c534c08710
  theStack:
    re-ACK c534c08710

Tree-SHA512: 08a0a422d2937eadcf0edfede37e535e6bc4c2e4b192441bbf9bc26dd3f03fa3388effd22f0527c55af173933d0b50e5b2b3d36f2b62d0aca3098728ef06970e
2023-11-07 16:48:57 -05:00
Jameson Lopp
9ac114e5cd
Throw error if invalid parameters passed to getnetworkhashps RPC endpoint 2023-11-07 12:58:42 -05:00
fanquake
49d953281d
fuzz: explicitly specify llvm-symbolizer path in runner
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in #28147.

Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
2023-11-07 16:57:23 +00:00
Sjors Provoost
43de4d3630
doc: fix typos
As found by lint-spelling.py using codespell 2.2.6.
2023-11-07 10:21:51 +09:00
Andrew Chow
0387ca0774
Merge bitcoin/bitcoin#28612: Test: followups to #27823
5ab6419f38 test: randomized perturbing in feature_init (L0la L33tz)
64b80d5c5b test: simplify feature_init (Fabian Jahr)

Pull request description:

  Fixes #28603

  Added suggested simplifications and implemented randomization

ACKs for top commit:
  theStack:
    Light ACK 5ab6419f38
  maflcko:
    lgtm ACK 5ab6419f38
  achow101:
    ACK 5ab6419f38

Tree-SHA512: e6f43eef7f8dd12c7fccbe437cb430dc9d383825d7ab2caa0382d061f88dec6d28522e1ec78f3f58f26d35cba93512fa21e330c48d06b1d8141a16f07050af5a
2023-11-06 16:57:39 -05:00
Sebastian Falbesoner
5e6bc6d830 test: remove custom rpc timeout for wallet_miniscript.py, reorder in test_runner 2023-11-05 23:54:02 +01:00
Sebastian Falbesoner
5039c346ca init: completely remove -zapwallettxes (remaining hidden option)
The `-zapwallettxes` functionality has been removed in v0.21.0
(see commit 3340dbadd3 / PR #19671),
with the parameter being kept as hidden option, to inform users via
an exit error that `abandontransaction` should be used instead.

As any guides that still suggest to use `-zapwallettxes` would refer to
a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x),
it is highly unlikely that the error caused by the option is still
relevant for any user, hence it seems fine to remove it now.
2023-11-03 20:00:44 +01:00
Roman Zeyde
7cb9367157
rpc: keep .cookie if it was not generated
Otherwise, starting bitcoind twice may cause the `.cookie`
file generated by the first instance to be deleted by the
second instance shutdown (after failing to obtain a lock).
2023-11-03 17:29:27 +02:00
Andrew Chow
d9007f51a7
Merge bitcoin/bitcoin#28762: MiniMiner changes for package linearization
d9cc99d04e [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a3788c [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24a3b [MiniMiner] track inclusion order and add Linearize() function (glozow)
004075963f [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c0ba [MiniMiner] make target_feerate optional (glozow)
5a83f55c96 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e630b2 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b79b2 [lint] update expected boost includes (glozow)

Pull request description:

  This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.

  - Allow using `MiniMiner` on transactions that aren't in the mempool.
  - Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
    - Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
  - Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
  - Tests

  Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.

ACKs for top commit:
  TheCharlatan:
    ACK d9cc99d04e
  kevkevinpal:
    reACK [d9cc99d](d9cc99d04e)
  achow101:
    re-ACK d9cc99d04e

Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
2023-11-03 10:50:50 -04:00
MarcoFalke
fa02598469
test: Add missing sync on send_version in peer_connect 2023-11-03 13:27:02 +01:00
glozow
4aa98b79b2 [lint] update expected boost includes 2023-11-03 10:17:41 +00:00
Andrew Chow
5f88622191
Merge bitcoin/bitcoin#27852: test: add coverage to rpc_blockchain.py
376dc2cfb3 test: add coverage to rpc_blockchain.py (kevkevin)

Pull request description:

  Included a test that checks the functionality of setting
  the first param of getnetworkhashps to negative value returns
  the average network hashes per second from the last difficulty change.

ACKs for top commit:
  jlopp:
    tACK 376dc2cfb3
  achow101:
    ACK 376dc2cfb3
  ismaelsadeeq:
    Tested ACK 376dc2cfb3
  pablomartin4btc:
    tACK 376dc2cfb3

Tree-SHA512: 02d52f622e9cb7a1240c5d124510dd75d03f696f119b2625b0befd60b004ec50ff1a2d5515e0e227601adeecd837e0778ed131ee2a8c5f75f1b824be711213a7
2023-11-02 15:19:37 -04:00
MarcoFalke
fa7d31910a
refactor: Remove unused circular include dependency from validation.cpp 2023-11-01 17:45:48 +01:00
fanquake
eca2e430ac
Merge bitcoin/bitcoin#28632: test: make python p2p not send getaddr on incoming connections
9cfc1c9440 test: check that we don't send a getaddr msg to an inbound peer (Martin Zumsande)
88c33c6748 test: make python p2p not send getaddr messages when it's being connected to (Martin Zumsande)

Pull request description:

  `bitcoind` nodes send `getaddr` messages only to outbound nodes (and ignore `getaddr` received by outgoing connections).
  The python p2p node should mirror this behavior by not sending a `getaddr` message when it is not the initiator of the connection.
  This is currently causing several unnecessary messages being sent and then ignored (`Ignoring "getaddr" from outbound-full-relay connection.`) in tests like `p2p_add_connections.py`.

ACKs for top commit:
  pinheadmz:
    concept ACK 9cfc1c9440
  pablomartin4btc:
    re ACK 9cfc1c9440
  BrandonOdiwuor:
    re ACK 9cfc1c9440

Tree-SHA512: 812bec5d8a4828b4384d4cdd4362d6eec09acb2363e888f2b3e3bf8b925e0e17f15e13dc297d6b616c68b93ace9ede7245b07b405d3f5f8eada98350f74230dc
2023-11-01 10:39:48 +00:00
fanquake
4733de3242
Merge bitcoin/bitcoin#28729: addrman: log AS only when using asmap
02a4f1a385 addrman: log AS only when using asmap (brunoerg)

Pull request description:

  This PR changes the log to just print the ASN when using asmap, same logic presented in other logs:

  afa081a39b/src/net_processing.cpp (L3552-L3556)

  afa081a39b/src/net_processing.cpp (L3598-L3604)

ACKs for top commit:
  naumenkogs:
    ACK 02a4f1a385
  mzumsande:
    Code Review ACK 02a4f1a385

Tree-SHA512: adad5904ab163660d47554b32dc2dc3dfdff8dd64b94e5320ad11706381264d1e338654fa8239430eed4ccbebc8f6670698b4278895794055c37fc4bcefe71bc
2023-11-01 10:25:53 +00:00
brunoerg
02a4f1a385 addrman: log AS only when using asmap 2023-10-30 18:46:06 -03:00
Sergi Delgado Segura
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently
Currently it is possible to add the same node twice when formatting IPs in
different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g:

127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1]

`addnode` will accept both and display both as connected (given they translate to
the same IP). This will not result in multiple connections to the same node, but
will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes`
with redundant data.

This can be avoided performing comparing the contents of `m_added_addr` and the address
to be added as `CServices` instead of as strings.
2023-10-30 11:39:19 -04:00
Brandon Odiwuor
897d6dd42b Remove feature_txindex_compatibility.py in V27 2023-10-30 13:35:12 +03:00
fanquake
feae4e0438
Merge bitcoin/bitcoin#28698: assumeutxo, blockstorage: Prevent core dump on invalid hash
811067ca1c test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc)
4a5be10b92 assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc)

Pull request description:

  While reviewing #27596 (ran `loadtxoutset` in `mainnet` before `m_assumeutxo_data` is empty as [currently](434495a8c1/src/kernel/chainparams.cpp (L175-L177)) in master  - back to 1b1d711), got a `core dumped`, so it seems there's a potential issue if new releases ever remove snapshot details or a semi-experienced user performs a `loadtxoutset` on a different "customised" binary version (not sure if this is a real use case).

  ```
  2023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
  node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed.
  Aborted (core dumped)
  ```

  <details>
  <summary>This is also happening before IBD is completed (<code>background validation</code> still being performed as it can be seen in rpc <code>getchainstates</code>)</summary>

  ```
  /src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
  {
    "headers": 813097,
    "chainstates": [
      {
        "blocks": 368249,
        "bestblockhash": "00000000000000000b7a08224a1cb00d337100ba7a46c03d04b2c2d8964efc37",
        "difficulty": 52278304845.59168,
        "verificationprogress": 0.086288278873286,
        "coins_db_cache_bytes": 7969177,
        "coins_tip_cache_bytes": 14908338995,
        "validated": true
      },
      {
        "blocks": 813097,
        "bestblockhash": "0000000000000000000270c9fdce7b17db64cca91f90106964b58e33a4d91089",
        "difficulty": 61030681983175.59,
        "verificationprogress": 0.999997140098457,
        "coins_db_cache_bytes": 419430,
        "coins_tip_cache_bytes": 784649420,
        "snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
        "validated": false
      }
    ]
  }
  ```
  </details>

  <details>
  <summary>Steps to reproduce the core dump error and its output:</summary>

  1. Perform a `loadtxoutset` in `mainnet` on compiled `bitcoind` adding the block hash from Sjors's [commit](24deb2022b).
  2. Once step 1 finishes, remove the added code from step 1 and compile again or just compile `master` without any changes on top.
  3. Run `bitcoind`, soon it'll crash with:

  ```
  2023-10-18T17:42:52Z [init] init message: Loading block index…
  2023-10-18T17:42:52Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures.
  2023-10-18T17:42:52Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64
  2023-10-18T17:42:52Z [init] Prune configured to target 3000 MiB on disk for block and undo files.
  2023-10-18T17:42:52Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading
  2023-10-18T17:42:52Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null)
  2023-10-18T17:42:52Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index
  2023-10-18T17:42:52Z [init] Opened LevelDB successfully
  2023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
  node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed.
  Aborted (core dumped)
  ```
  </details>

  <details>
  <summary>After original change, error message output:</summary>

  ```
  2023-10-20T15:49:12Z [init] init message: Loading block index…
  2023-10-20T15:49:12Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures.
  2023-10-20T15:49:12Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64
  2023-10-20T15:49:12Z [init] Prune configured to target 3000 MiB on disk for block and undo files.
  2023-10-20T15:49:12Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading
  2023-10-20T15:49:12Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null)
  2023-10-20T15:49:12Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index
  2023-10-20T15:49:12Z [init] Opened LevelDB successfully
  2023-10-20T15:49:12Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
  2023-10-20T15:49:13Z [init] *** Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
  2023-10-20T15:49:13Z [init] Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
  Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
  2023-10-20T15:49:13Z [init] Shutdown requested. Exiting.
  2023-10-20T15:49:13Z [init] Shutdown: In progress...
  2023-10-20T15:49:13Z [scheduler] scheduler thread exit
  2023-10-20T15:49:13Z [shutoff] Flushed fee estimates to fee_estimates.dat.
  2023-10-20T15:49:13Z [shutoff] Shutdown: done
  ```
  </details>

  <details>
  <summary>Alternative on error handling using <code>return error()</code> instead of <code>return FatalError()</code> used in this PR, which produces a different output and perhaps confusing:</summary>

  ```
  2023-10-20T21:45:58Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
  2023-10-20T21:45:59Z [init] ERROR: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
  2023-10-20T21:45:59Z [init] : Error loading block database.
  Please restart with -reindex or -reindex-chainstate to recover.
  : Error loading block database.
  Please restart with -reindex or -reindex-chainstate to recover.
  2023-10-20T21:45:59Z [init] Aborted block database rebuild. Exiting.
  2023-10-20T21:45:59Z [init] Shutdown: In progress...
  2023-10-20T21:45:59Z [scheduler] scheduler thread exit
  2023-10-20T21:45:59Z [shutoff] Flushed fee estimates to fee_estimates.dat.
  2023-10-20T21:45:59Z [shutoff] Shutdown: done
  ```

  </details>

  <details>
  <summary>Current state (including ryanofsky <a href="https://github.com/bitcoin/bitcoin/pull/28698#discussion_r1368635965">suggestion</a>), after code change, error message output:</summary>

  ```
  2023-10-25T02:29:57Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000
  2023-10-25T02:29:57Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
  2023-10-25T02:29:57Z [init] Error: A fatal internal error occurred, see debug.log for details
  Error: A fatal internal error occurred, see debug.log for details
  2023-10-25T02:29:57Z [init] Shutdown requested. Exiting.
  2023-10-25T02:29:57Z [init] Shutdown: In progress...
  2023-10-25T02:29:57Z [scheduler] scheduler thread exit
  2023-10-25T02:29:57Z [shutoff] Flushed fee estimates to fee_estimates.dat.
  2023-10-25T02:29:57Z [shutoff] Shutdown: done
  ```

  </details>

ACKs for top commit:
  naumenkogs:
    ACK 811067ca1c
  theStack:
    ACK 811067ca1c
  ryanofsky:
    Code review ACK 811067ca1c.

Tree-SHA512: cfc137b0a4f638b99fd7dac2c35cc729ef71ae1166a2a8960a91055ec90841cb33aed589834012cfe0e157937e2a76a88d1020ea1df2bc98e1114eb1fc8eaae4
2023-10-29 10:22:10 +01:00
fanquake
f028470654
Merge bitcoin/bitcoin#28727: test: replace random_bytes with random.randbytes
fe3ac3700d test: replace random_bytes with randbytes #28720 (ns-xvrn)

Pull request description:

  With Python upgraded to 3.9 replaced the `random_bytes` function in util of functional tests and replaced it's usage with `random.randbytes`.

  Closes #28720.

ACKs for top commit:
  maflcko:
    lgtm ACK fe3ac3700d
  BrandonOdiwuor:
    ACK fe3ac3700d
  stickies-v:
    ACK fe3ac3700d, thanks for picking this up
  kristapsk:
    utACK fe3ac3700d

Tree-SHA512: f65a75e73ebd840c2936eb133d42bccd552f25b717c8ca25c18d06e0593e12f292389cfcc0a0b0759004b67a46ea0c8ac237973ef90f246139778230be1e64e1
2023-10-29 10:14:59 +01:00
Andrew Chow
2a349f9ea5
Merge bitcoin/bitcoin#28264: test: refactor: support sending funds with outpoint result
50d1ac1207 test: remove unused `find_output` helper (Sebastian Falbesoner)
73a339abc3 test: refactor: support sending funds with outpoint result (Sebastian Falbesoner)

Pull request description:

  In wallet-related functional tests we often want to send funds to an address and  use the resulting (non-change) UTXO directly after as input for another transaction. Doing that is currently tedious, as it involves finding the index part of the outpoint manually by calling helpers like `find_vout_for_address` or `find_output` first.  This results in two different txid/vout variables which then again have to be combined to a single dictionary `{"txid": ..., "vout": ...}` in order to be specified as input for RPCs like `createrawtransaction` or `createpsbt`. For example:

  ```
  txid1 = node1.sendtoaddress(addr1, value1)
  vout1 = find_vout_for_address(node1, txid1, addr1)
  txid2 = node2.sendtoaddress(addr2, value2)
  vout2 = find_vout_for_address(node2, txid2, addr2)
  node.createrawtransaction([{'txid': txid1, 'vout': vout1}, {'txid': txid2, 'vout': vout2}], .....)
  ```

  This PR introduces a helper `create_outpoints` to immediately return the outpoint as
  UTXO dictionary in the common format, making the tests more readable and avoiding unnecessary duplication:

  ```
  utxo1 = self.create_outpoints(node1, outputs=[{addr1: value1}])[0]
  utxo2 = self.create_outpoints(node2, outputs=[{addr2: value2}])[0]
  node.createrawtransaction([utxo1, utxo2], .....)
  ```

  Tests are switched to work with UTXO-objects rather than two individual txid/vout variables accordingly.

  The `find_output` helper is removed, as it seems generally a bad idea to search for an outpoint only based on the output value. If that's really ever needed in the future, it makes probably more sense to add it as an additional parameter to `find_vout_of_address`. Note that `find_output` supported specifying a block-hash for where to look for the transaction (being passed on to the `getrawtransaction` RPC). This seems to be unneeded, as txids are always unique and for the only test that used that parameter (rpc_psbt.py) there was no observed difference in run-time, so it was not reintroduced in the new helper.

  There are still some `find_vout_of_address` calls remaining, used for detecting change outputs or for whenever the sending happens via `sendrawtransaction` instead, so this PR tackles not all, but the most common case.

ACKs for top commit:
  achow101:
    ACK 50d1ac1207
  BrandonOdiwuor:
    ACK 50d1ac1207
  maflcko:
    ACK 50d1ac1207 🖨

Tree-SHA512: af2bbf13a56cc840fefc1781390cf51625f1e41b3c030f07fc9abb1181b2d414ddbf795e887db029e119cbe45de14f7c987c0cba72ff0b8953080ee218a7915a
2023-10-25 11:05:38 -04:00
pablomartin4btc
811067ca1c test: add coverage for snapshot chainstate not matching AssumeUTXO parameters
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
2023-10-25 11:14:43 -03:00
ns-xvrn
fe3ac3700d test: replace random_bytes with randbytes #28720 2023-10-25 08:56:41 -04:00
Hennadii Stepanov
64879f4c03
Merge bitcoin-core/gui#771: Avoid error-prone leading whitespace in translatable strings
856325fac1 lint: Add `lint-qt-translation.py` (Hennadii Stepanov)
294a018bf5 qt: Avoid error prone leading spaces in translatable strings (Hennadii Stepanov)
d8298e7f06 qt, refactor: Drop superfluous type conversions (Hennadii Stepanov)

Pull request description:

  While working on the GUI translation via Transifex web interface, I found it error-prone to have leading whitespace in translatable strings. This is because it is very easy to unintentionally drop them in translations unnoticed.

  Fixed all current cases. Added a linter to prevent similar cases in the future.

ACKs for top commit:
  furszy:
    utACK 856325f

Tree-SHA512: b1ca5effb2db6649e1e99382de79acf3a9f81cc9dad434db5623338489e597897e8addd60c1ab3dcc7506ae62753a7a4ad5a41d7a865f8fcdf94348b54baa7e7
2023-10-25 13:20:07 +01:00