💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551574)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551574)
Done.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551654)
Good call, done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551654)
Good call, done.
💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898331957)
> If we can avoid to have a split function at all that would seem great.
We can't avoid splitting when calling `execvp`:https://github.com/bitcoin/bitcoin/blob/87ec923d3a7af7b30613174b41c6fb11671df466/src/util/subprocess.h#L1382
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898331957)
> If we can avoid to have a split function at all that would seem great.
We can't avoid splitting when calling `execvp`:https://github.com/bitcoin/bitcoin/blob/87ec923d3a7af7b30613174b41c6fb11671df466/src/util/subprocess.h#L1382
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100561289)
Just varying the amounts because why not.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100561289)
Just varying the amounts because why not.
💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100610295)
> In [1e826bf](https://github.com/bitcoin/bitcoin/commit/1e826bf91932e2d76969ae40a0c0ef85a990cf2f): Can the commit be updated to better explain why this is needed. Looking at mingw-w64, `GetACP` has returned `CP_UTF8` since it was introduced. I'm guessing the same is happening with MSVC (although that's less relevant given it doesn't effect releases)?
`GetACP` being compiled on Windows returns 1252, which is not the UT8 code page. And, basically, it would depend on the OS language settings.
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100610295)
> In [1e826bf](https://github.com/bitcoin/bitcoin/commit/1e826bf91932e2d76969ae40a0c0ef85a990cf2f): Can the commit be updated to better explain why this is needed. Looking at mingw-w64, `GetACP` has returned `CP_UTF8` since it was introduced. I'm guessing the same is happening with MSVC (although that's less relevant given it doesn't effect releases)?
`GetACP` being compiled on Windows returns 1252, which is not the UT8 code page. And, basically, it would depend on the OS language settings.
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100618966)
It's confusing, but otherwise harmless.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100618966)
It's confusing, but otherwise harmless.
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2898436841)
ACK 80d213677962268a82d65d8915c518f0c02867d9
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2898436841)
ACK 80d213677962268a82d65d8915c518f0c02867d9
💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100633125)
> Looking at mingw-w64, `GetACP` has returned `CP_UTF8` since it was introduced.
I see it differently. I've cross-compiled this code:
```c++
#include <iostream>
#include <windows.h>
int main()
{
std::cout << GetACP() << "\n";
}
```
and it prints 1252 on Windows.
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100633125)
> Looking at mingw-w64, `GetACP` has returned `CP_UTF8` since it was introduced.
I see it differently. I've cross-compiled this code:
```c++
#include <iostream>
#include <windows.h>
int main()
{
std::cout << GetACP() << "\n";
}
```
and it prints 1252 on Windows.
👍 hebasto approved a pull request: "fs: remove `_POSIX_C_SOURCE` defining"
(https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2858336344)
re-ACK 24e5fd3bedcebacbc10f0449be61be636b77dd79, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748).
(https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2858336344)
re-ACK 24e5fd3bedcebacbc10f0449be61be636b77dd79, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748).
💬 ryanofsky commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898557463)
> We can't avoid splitting when calling `execvp`:
The suggestion is to let the shell do the splitting so we do not need to do it. We would call execvp with `sh` as the first command line argument `-c` as the second one, and the -signer value as the third one. This is also what the python subprocess module does when shell=True is passed.
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898557463)
> We can't avoid splitting when calling `execvp`:
The suggestion is to let the shell do the splitting so we do not need to do it. We would call execvp with `sh` as the first command line argument `-c` as the second one, and the -signer value as the third one. This is also what the python subprocess module does when shell=True is passed.
💬 i-am-yuvi commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2898614400)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2898614400)
Concept ACK
🤔 i-am-yuvi reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858508507)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858508507)
Concept ACK
🤔 i-am-yuvi reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858551944)
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
- `DisconnectTip()` & reconnect via `invalidateblock` and `reconsiderblock`
- `LimitMemoryUsage()`: exercised by flooding > 20 MB
- `test_chainlimits_exceeded`
Logs
```
2025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
2025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
2025-05-21T16:57:42.337000Z TestFr
...
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2858551944)
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
- `DisconnectTip()` & reconnect via `invalidateblock` and `reconsiderblock`
- `LimitMemoryUsage()`: exercised by flooding > 20 MB
- `test_chainlimits_exceeded`
Logs
```
2025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
2025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
2025-05-21T16:57:42.337000Z TestFr
...
📝 dergoegge opened a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
iff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cp
...
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
iff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cp
...
👋 rkrux's pull request is ready for review: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580)
(https://github.com/bitcoin/bitcoin/pull/32580)
📝 dergoegge converted_to_draft a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.c
...
(https://github.com/bitcoin/bitcoin/pull/32581)
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.c
...
💬 dergoegge commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2898738831)
Draft until I sort out the CI failures
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2898738831)
Draft until I sort out the CI failures
💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2898747996)
> It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
I did the follow-ups in #32580.
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2898747996)
> It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
I did the follow-ups in #32580.
💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898773591)
Rebased, ready for review.
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-2898773591)
Rebased, ready for review.
👋 achow101's pull request is ready for review: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523)
(https://github.com/bitcoin/bitcoin/pull/32523)