💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2821309098)
Although `test_direct_file` doesn't currently fail, adding a check for the wallet's name in the backup path does make it fail, e.g.
That's a good point. Instead of backing up `plainfile` as `plainfile_123.legacy.bak` 47174476c04a7f1138fbd32eb0c9c38e85946393 backs it up as `wallets_123.legacy.bak` because it assumes the name of the parent directory is the name of the wallet. Following would seem to be a good fix:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4499,13
...
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2821309098)
Although `test_direct_file` doesn't currently fail, adding a check for the wallet's name in the backup path does make it fail, e.g.
That's a good point. Instead of backing up `plainfile` as `plainfile_123.legacy.bak` 47174476c04a7f1138fbd32eb0c9c38e85946393 backs it up as `wallets_123.legacy.bak` because it assumes the name of the parent directory is the name of the wallet. Following would seem to be a good fix:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4499,13
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2054107249)
> This is needed to be able to read without obfuscation - as the comment states.
Why does it need to be set to zero again when it's been set in the initializer list of this same ctor?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2054107249)
> This is needed to be able to read without obfuscation - as the comment states.
Why does it need to be set to zero again when it's been set in the initializer list of this same ctor?
💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054112074)
It feels out of place in mining_basic, and the PR that introduces validation checks for nSequence/nLockTime in coinbase does check various combinations in feature_block, including this one. So i think it's better left there?
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054112074)
It feels out of place in mining_basic, and the PR that introduces validation checks for nSequence/nLockTime in coinbase does check various combinations in feature_block, including this one. So i think it's better left there?
💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054128939)
Done.
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054128939)
Done.
💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054129498)
Sure, done.
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054129498)
Sure, done.
💬 fanquake commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821359451)
I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821359451)
I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?
💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821394743)
> I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?
Was related to LTO?
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821394743)
> I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?
Was related to LTO?
💬 thesamesam commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821405387)
It's the same problem. It just shows up worse with LTO because Qt's sanity check can't fire to tell you and give you a build failure instead.
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821405387)
It's the same problem. It just shows up worse with LTO because Qt's sanity check can't fire to tell you and give you a build failure instead.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2821410082)
Changed the serialization of `Obfuscation` from `vector` -> `array` without re-testing my suggestions towards the end. Forgot that one serializes the size and the other does not. Seems to be responsible for some of the test failures on my suggestion-branch.
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2821410082)
Changed the serialization of `Obfuscation` from `vector` -> `array` without re-testing my suggestions towards the end. Forgot that one serializes the size and the other does not. Seems to be responsible for some of the test failures on my suggestion-branch.
💬 maflcko commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821488921)
Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821488921)
Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.
💬 hebasto commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821506859)
> Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.
The question is how to choose the configuration for testing.
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821506859)
> Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.
The question is how to choose the configuration for testing.
📝 maflcko opened a pull request: "ci: Add missing -Wno-error=array-bounds to valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/32325)
Due to an upstream GCC issue, any debug/fuzz build which aborts on failed assumes will print a false positive array-bounds warning in `src/test/fuzz/txgraph.cpp`.
This also affects one CI task.
Fix the CI task by ignoring the error for now.
Fixes https://github.com/bitcoin/bitcoin/issues/32276
(https://github.com/bitcoin/bitcoin/pull/32325)
Due to an upstream GCC issue, any debug/fuzz build which aborts on failed assumes will print a false positive array-bounds warning in `src/test/fuzz/txgraph.cpp`.
This also affects one CI task.
Fix the CI task by ignoring the error for now.
Fixes https://github.com/bitcoin/bitcoin/issues/32276
🤔 furszy reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2784336700)
utACK cad39f86fb5a81
> Sounds like this should be an actual test then instead of a benchmark, no?
I'd keep it as is for now and drive all our focus towards #31250. It would be nice to cut down the tree first - we can pick up any leftover later.
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2784336700)
utACK cad39f86fb5a81
> Sounds like this should be an actual test then instead of a benchmark, no?
I'd keep it as is for now and drive all our focus towards #31250. It would be nice to cut down the tree first - we can pick up any leftover later.
💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2821541818)
I've written a tool to check translations: https://github.com/maflcko/DrahtBot/tree/main/check_translations
This finds the above issues, as well as one more:
```
$ cargo run -- --llm-api-key hi --translation-file /path/to/src/qt/locale/bitcoin_pl.ts --cache-dir cache
Erroneous translation:
<source>Create Wallet</source>
<translation type="unfinished">Stwórz potrfel</translation>
Erroneous translation:
<source>%1 (%2 blocks)</source>
<translation type="
...
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2821541818)
I've written a tool to check translations: https://github.com/maflcko/DrahtBot/tree/main/check_translations
This finds the above issues, as well as one more:
```
$ cargo run -- --llm-api-key hi --translation-file /path/to/src/qt/locale/bitcoin_pl.ts --cache-dir cache
Erroneous translation:
<source>Create Wallet</source>
<translation type="unfinished">Stwórz potrfel</translation>
Erroneous translation:
<source>%1 (%2 blocks)</source>
<translation type="
...
📝 vasild opened a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2054291997)
Thanks for the input, I respect and agree with most of it. The only bit I do not agree with is a general rule to avoid `shared_ptr`. I agree using `unique_ptr` is better than `shared_ptr`, especially if `unique_ptr` is to be used as intented - to manage the lifetime of the owned object. If we start extracting the raw pointers from `unique_ptr` and managing them separately and manually ref-counting then maybe it is better to just use `shared_ptr`.
> FindNode() returns shared_ptr, this can be c
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2054291997)
Thanks for the input, I respect and agree with most of it. The only bit I do not agree with is a general rule to avoid `shared_ptr`. I agree using `unique_ptr` is better than `shared_ptr`, especially if `unique_ptr` is to be used as intented - to manage the lifetime of the owned object. If we start extracting the raw pointers from `unique_ptr` and managing them separately and manually ref-counting then maybe it is better to just use `shared_ptr`.
> FindNode() returns shared_ptr, this can be c
...
💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054294507)
style nit in 1da11dbc441790773502ffd5f60dc05191514a83: It would be good to use clang-format. This would reduce the whitespace churn of this commit.
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2054294507)
style nit in 1da11dbc441790773502ffd5f60dc05191514a83: It would be good to use clang-format. This would reduce the whitespace churn of this commit.
💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2821598466)
review ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718 🍥
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK cad39f86fb5a
...
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2821598466)
review ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718 🍥
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK cad39f86fb5a
...
💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054309204)
This rename makes obvious that maybe something is wrong in the current code in `master`. Is the intention really to ignore inbound connections when, according to the comment we intent to "Look for an existing connection"?
For example, if `1.1.1.1` connects to `2.2.2.2:8333`, then the TCP connection will look like:
`1.1.1.1:53481 <-> 2.2.2.2:8333` where `53481` is a random port picked by the initiator's OS. Now, if `2.2.2.2` intends to connect to `1.1.1.1:8333` then this check will return fal
...
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054309204)
This rename makes obvious that maybe something is wrong in the current code in `master`. Is the intention really to ignore inbound connections when, according to the comment we intent to "Look for an existing connection"?
For example, if `1.1.1.1` connects to `2.2.2.2:8333`, then the TCP connection will look like:
`1.1.1.1:53481 <-> 2.2.2.2:8333` where `53481` is a random port picked by the initiator's OS. Now, if `2.2.2.2` intends to connect to `1.1.1.1:8333` then this check will return fal
...
💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054324859)
In 9e657358271e7a045c, to make the CI happy
```suggestion
const CNetAddr& net_addr{addr};
```
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054324859)
In 9e657358271e7a045c, to make the CI happy
```suggestion
const CNetAddr& net_addr{addr};
```