🤔 maflcko reviewed a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-3006435253)
the rebase looks correct. (ci obviously still fails, as expected)
re-ACK 605593547aa196204b1e5b8f7857abacd36906b2 🏣
<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+krxU1A3Yux4bpwZ
...
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-3006435253)
the rebase looks correct. (ci obviously still fails, as expected)
re-ACK 605593547aa196204b1e5b8f7857abacd36906b2 🏣
<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+krxU1A3Yux4bpwZ
...
💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r2198179128)
nit in 8f22dd5e4c4b116ad873e464ea7f75caaef3476d: Would be nice to use the same logic flow like above in this test:
```py
self.log.info("Verify warning is emitted when failing to scan the wallets directory")
if platform.system() == 'Windows':
self.log.warning('Skipping test involving chmod as Windows does not support it.')
elif os.geteuid() == 0:
self.log.warning('Skipping test involving chmod as it requires a non-root user.')
else:
...
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r2198179128)
nit in 8f22dd5e4c4b116ad873e464ea7f75caaef3476d: Would be nice to use the same logic flow like above in this test:
```py
self.log.info("Verify warning is emitted when failing to scan the wallets directory")
if platform.system() == 'Windows':
self.log.warning('Skipping test involving chmod as Windows does not support it.')
elif os.geteuid() == 0:
self.log.warning('Skipping test involving chmod as it requires a non-root user.')
else:
...
💬 fanquake commented on issue "cmake: searching across directories for config files":
(https://github.com/bitcoin/bitcoin/issues/32938#issuecomment-3058124124)
That seems to fix it. I haven't tested extensively though.
(https://github.com/bitcoin/bitcoin/issues/32938#issuecomment-3058124124)
That seems to fix it. I haven't tested extensively though.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2198190875)
Removed
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2198190875)
Removed
💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3058141540)
@hebasto The logo sizes remain unchanged; only the margins have been removed. When compiling Bitcoin Core, everything appears exactly as it did before.
The shadows were removed to match the original Bitcoin logo from bitboy, which also did not include shadows. Since users may download the Bitcoin logo (example the SVG version) directly from Bitcoin Core's source code, this ensures they receive properly optimized logo files.
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3058141540)
@hebasto The logo sizes remain unchanged; only the margins have been removed. When compiling Bitcoin Core, everything appears exactly as it did before.
The shadows were removed to match the original Bitcoin logo from bitboy, which also did not include shadows. Since users may download the Bitcoin logo (example the SVG version) directly from Bitcoin Core's source code, this ensures they receive properly optimized logo files.
💬 hebasto commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3058183303)
GitHub renders chnages as follows:
<img width="541" height="330" alt="image" src="https://github.com/user-attachments/assets/9100a63c-334d-47bd-8b65-73433092dc2d" />
> ... only the margins have been removed.
Why?
> The shadows were removed to match the original Bitcoin logo from bitboy, which also did not include shadows.
Bitcoin Core has its own logo for years. I don't see your reference as a justification for this change.
> Since users may download the Bitcoin logo (example
...
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3058183303)
GitHub renders chnages as follows:
<img width="541" height="330" alt="image" src="https://github.com/user-attachments/assets/9100a63c-334d-47bd-8b65-73433092dc2d" />
> ... only the margins have been removed.
Why?
> The shadows were removed to match the original Bitcoin logo from bitboy, which also did not include shadows.
Bitcoin Core has its own logo for years. I don't see your reference as a justification for this change.
> Since users may download the Bitcoin logo (example
...
💬 tnndbtc commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058210889)
Thanks for @Sjors ' quick clue on the potential issue. I searched the CI output and agree that it's likely due to the calling of
`return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
`
which requires a read from stdin in `test/functional/mocks/signer.py`
```
if not sys.stdin.isatty(): # on my local mac test, this is not from terminal so it will try to run read()
buffer = sys.stdin.read() # this
...
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058210889)
Thanks for @Sjors ' quick clue on the potential issue. I searched the CI output and agree that it's likely due to the calling of
`return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
`
which requires a read from stdin in `test/functional/mocks/signer.py`
```
if not sys.stdin.isatty(): # on my local mac test, this is not from terminal so it will try to run read()
buffer = sys.stdin.read() # this
...
💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3058232808)
> @HowHsu, check this test [furszy@f2b8a06](https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38). It triggers the reorg assertion failure without needing to deduplicate the index synchronization code. The test fails without your fix commit and passes with it. Feel free to cherry-pick it.
>
> > The Sync in the test is same as in bitcoind, expect the thread synchronization code which is to trigger the reorg. The only cons is we have to keep it updated when the
...
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3058232808)
> @HowHsu, check this test [furszy@f2b8a06](https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38). It triggers the reorg assertion failure without needing to deduplicate the index synchronization code. The test fails without your fix commit and passes with it. Feel free to cherry-pick it.
>
> > The Sync in the test is same as in bitcoind, expect the thread synchronization code which is to trigger the reorg. The only cons is we have to keep it updated when the
...
📝 stickies-v opened a pull request: "rest/rpc: use more util::Join"
(https://github.com/bitcoin/bitcoin/pull/32942)
There are a few places in the RPC and REST code where we manually join multiple items into a string. This PR removes a bunch of that logic and replaces it with `util::Join`. To minimize unnecessary container allocations and simplify implementations, the first commit slightly refactors `util::Join` so it can subsequently be used with range adaptors.
Tangentially, since we're touching the code anyway, refactor and modernize `rf_names` into a `std::map`.
(https://github.com/bitcoin/bitcoin/pull/32942)
There are a few places in the RPC and REST code where we manually join multiple items into a string. This PR removes a bunch of that logic and replaces it with `util::Join`. To minimize unnecessary container allocations and simplify implementations, the first commit slightly refactors `util::Join` so it can subsequently be used with range adaptors.
Tangentially, since we're touching the code anyway, refactor and modernize `rf_names` into a `std::map`.
👍 instagibbs approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3006617887)
ACK https://github.com/bitcoin/bitcoin/pull/32587/commits/4ddc577953a7407ea44a3e3d8d9a4635929307ca
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3006617887)
ACK https://github.com/bitcoin/bitcoin/pull/32587/commits/4ddc577953a7407ea44a3e3d8d9a4635929307ca
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2198282402)
mempool is already clean?
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2198282402)
mempool is already clean?
🤔 stickies-v reviewed a pull request: "rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF`"
(https://github.com/bitcoin/bitcoin/pull/32884#pullrequestreview-3006657167)
post-merge ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
Looking at this code inspired me to make some related modernizations to the `rf_names` code: https://github.com/bitcoin/bitcoin/pull/32942
(https://github.com/bitcoin/bitcoin/pull/32884#pullrequestreview-3006657167)
post-merge ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
Looking at this code inspired me to make some related modernizations to the `rf_names` code: https://github.com/bitcoin/bitcoin/pull/32942
🤔 monlovesmango reviewed a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-3006458828)
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
The separation of concerns in the cluster linearization fuzz tests makes sense, namely separating testing of production of code from testing of test code. The small added efficiencies and comparisons are nice and the diagram documenting all the relationships is extremely helpful in clarifying how all the parts tie together.
Unrelated to this specific PR, but in the `clusterlin_postlinearize_tree` fuzz target, `post_post_linearization` isn't actu
...
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-3006458828)
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
The separation of concerns in the cluster linearization fuzz tests makes sense, namely separating testing of production of code from testing of test code. The small added efficiencies and comparisons are nice and the diagram documenting all the relationships is extremely helpful in clarifying how all the parts tie together.
Unrelated to this specific PR, but in the `clusterlin_postlinearize_tree` fuzz target, `post_post_linearization` isn't actu
...
💬 monlovesmango commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2198250370)
I think this comment can be resolved because `clusterlin_simple_linearize` now has a check for `simple_chunking` against `read_chunking` in optimal scenario?
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2198250370)
I think this comment can be resolved because `clusterlin_simple_linearize` now has a check for `simple_chunking` against `read_chunking` in optimal scenario?
💬 monlovesmango commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2198192109)
Should `SearchCandidateFinder` also point to `AncestorCandidateFinder` (with single arrow ->) since it is also used in `clusterlin_search_finder` test? Similar to how `SimpleCandidateFinder` implementation is tested with both `ExhaustiveCandidateFinder` and `AncestorCandidateFinder` in `clusterlin_simple_finder`?
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2198192109)
Should `SearchCandidateFinder` also point to `AncestorCandidateFinder` (with single arrow ->) since it is also used in `clusterlin_search_finder` test? Similar to how `SimpleCandidateFinder` implementation is tested with both `ExhaustiveCandidateFinder` and `AncestorCandidateFinder` in `clusterlin_simple_finder`?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2198327882)
I still think we should be LimitOphan-ing for each AddAnnouncer?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2198327882)
I still think we should be LimitOphan-ing for each AddAnnouncer?
💬 maflcko commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058387306)
> Likely the stdin is missing in this particular run and caused `test/functional/mocks/signer.py` to hang.
What does "stdin is missing" mean? It should always be present and point to some file descriptor.
I think the only way this could hang is when the process spawning the signer is also waiting on it, without first closing the pipes. However this seems to happen correctly, looking at `RunCommandParseJSON`:
```cpp
auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE},
...
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058387306)
> Likely the stdin is missing in this particular run and caused `test/functional/mocks/signer.py` to hang.
What does "stdin is missing" mean? It should always be present and point to some file descriptor.
I think the only way this could hang is when the process spawning the signer is also waiting on it, without first closing the pipes. However this seems to happen correctly, looking at `RunCommandParseJSON`:
```cpp
auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE},
...
💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2198342677)
I removed `SetFresh` in the last commit. This lets us remove these tests.
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r2198342677)
I removed `SetFresh` in the last commit. This lets us remove these tests.
🚀 glozow merged a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605)
(https://github.com/bitcoin/bitcoin/pull/30605)
💬 maflcko commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058393000)
Maybe this is https://github.com/bitcoin/bitcoin/issues/32524, but with N=1 and no way to reproduce, this seems difficult to debug.
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3058393000)
Maybe this is https://github.com/bitcoin/bitcoin/issues/32524, but with N=1 and no way to reproduce, this seems difficult to debug.