💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100476656)
thanks, done
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100476656)
thanks, done
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100477997)
No, 4MB! Great catch thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100477997)
No, 4MB! Great catch thanks, fixed.
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100481900)
awesome yes, thanks, done
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100481900)
awesome yes, thanks, done
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100488438)
yep thanks
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100488438)
yep thanks
📝 rkrux opened a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580)
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide cl
...
(https://github.com/bitcoin/bitcoin/pull/32580)
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide cl
...
💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2100509926)
What would you suggest as a test case?
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2100509926)
What would you suggest as a test case?
💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2898284120)
Reverted to the prior change, given we cannot assume availablility.
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2898284120)
Reverted to the prior change, given we cannot assume availablility.
💬 ryanofsky commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898288496)
I'd think for both windows and unix, a good approach could be not parse -signer strings, just leave it up to the shell on unix, and shell+appilcation itself on windows (because on windows the application runtime is responsible for splitting arguments, usually using [CommandLineToArgv](https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw)) function. (This was also discussed pretty extensively in https://github.com/bitcoin/bitcoin/pull/13339)
If we can avo
...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898288496)
I'd think for both windows and unix, a good approach could be not parse -signer strings, just leave it up to the shell on unix, and shell+appilcation itself on windows (because on windows the application runtime is responsible for splitting arguments, usually using [CommandLineToArgv](https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw)) function. (This was also discussed pretty extensively in https://github.com/bitcoin/bitcoin/pull/13339)
If we can avo
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551171)
Changed to "non-witness" and actually pushed it this time. :)
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551171)
Changed to "non-witness" and actually pushed it this time. :)
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551433)
Meh. Then it would not make it consistent with MAX_STD_P2SH_SIGOPS, and having in the name that's it's a standardness rule is useful. I don't think it's worth changing.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100551433)
Meh. Then it would not make it consistent with MAX_STD_P2SH_SIGOPS, and having in the name that's it's a standardness rule is useful. I don't think it's worth changing.
💬 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.