📝 davidgumberg opened a pull request: "test: sock: Enable socket pair tests on Windows"
(https://github.com/bitcoin/bitcoin/pull/33506)
Some `class Sock` tests were previously disabled because Windows lacks [`socketpair(2)`](https://man7.org/linux/man-pages/man2/socketpair.2.html). This PR adds a `CreateSocketPair()` helper for Windows which imitates [`socketpair()`](https://github.com/torvalds/linux/blob/1896ce8eb6c61824f6c1125d69d8fda1f44a22f8/net/unix/af_unix.c#L1830-L1859), and enables these test cases for Windows.
This change is generally an improvement, but is also broken out of a [branch](github.com/davidgumberg/bitc
...
(https://github.com/bitcoin/bitcoin/pull/33506)
Some `class Sock` tests were previously disabled because Windows lacks [`socketpair(2)`](https://man7.org/linux/man-pages/man2/socketpair.2.html). This PR adds a `CreateSocketPair()` helper for Windows which imitates [`socketpair()`](https://github.com/torvalds/linux/blob/1896ce8eb6c61824f6c1125d69d8fda1f44a22f8/net/unix/af_unix.c#L1830-L1859), and enables these test cases for Windows.
This change is generally an improvement, but is also broken out of a [branch](github.com/davidgumberg/bitc
...
💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389620468)
"support" doesn't help me understand this:
> support
To bear the weight of, especially from below; keep from falling, sinking, or slipping.
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389620468)
"support" doesn't help me understand this:
> support
To bear the weight of, especially from below; keep from falling, sinking, or slipping.
💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389636212)
I was just referring to the concerns of other reviewers about modifying production code - maybe a test could make sure we don't accidentally include this in the future. We have tests that start up `perf` tool, it shouldn't be too difficult to check if a debugger can attach to us in a test (though I wouldn't guard, I would want that test to fail if run with a debugger, that's how I would trust it) - but if you don't think it's useful, I understand.
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389636212)
I was just referring to the concerns of other reviewers about modifying production code - maybe a test could make sure we don't accidentally include this in the future. We have tests that start up `perf` tool, it shouldn't be too difficult to check if a debugger can attach to us in a test (though I wouldn't guard, I would want that test to fail if run with a debugger, that's how I would trust it) - but if you don't think it's useful, I understand.
💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389639009)
> add a functional test that runs bitcoind to confirm that `-waitfordebugger` triggers this error message
Yes, it would be reassuring to me that it's not on by default.
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389639009)
> add a functional test that runs bitcoind to confirm that `-waitfordebugger` triggers this error message
Yes, it would be reassuring to me that it's not on by default.
💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389640672)
Ah, so it's not the negative that we're guarding, in that case maybe:
```suggestion
attached = pid != 0;
```
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389640672)
Ah, so it's not the negative that we're guarding, in that case maybe:
```suggestion
attached = pid != 0;
```
📝 polespinasa opened a pull request: "RPC: add sendrawtransactiontopeer"
(https://github.com/bitcoin/bitcoin/pull/33507)
Adds a new RPC `sendrawtransactiontopeer`, which sends a single tx to a specific peer.
After the transaction is sent to the peer it is forgotten and not stored in the local mempool.
This rpc can serve several purposes. For example, it allows you to “spoof” an initial retransmission from a different "trusted" peer, making it appear as if the original participant was never aware of the transaction. It can also be useful in testing and simulation environments.
Solves #28636 and #21876 (P
...
(https://github.com/bitcoin/bitcoin/pull/33507)
Adds a new RPC `sendrawtransactiontopeer`, which sends a single tx to a specific peer.
After the transaction is sent to the peer it is forgotten and not stored in the local mempool.
This rpc can serve several purposes. For example, it allows you to “spoof” an initial retransmission from a different "trusted" peer, making it appear as if the original participant was never aware of the transaction. It can also be useful in testing and simulation environments.
Solves #28636 and #21876 (P
...
💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659427)
Even if the tx is already in the mempool `m_result_type` is never `ResultType::MEMPOOL_ENTRY` making it throw a `JSONTransactionError`.
This can be seen in the last test case in 5e4b8120af266026cfbc3ca29fc98ebb4fcf8f55.
This should not be the behavior, but I didn't find a solution yet.
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659427)
Even if the tx is already in the mempool `m_result_type` is never `ResultType::MEMPOOL_ENTRY` making it throw a `JSONTransactionError`.
This can be seen in the last test case in 5e4b8120af266026cfbc3ca29fc98ebb4fcf8f55.
This should not be the behavior, but I didn't find a solution yet.
💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659844)
This test case should be deleted as this is not the expected behavior, see https://github.com/bitcoin/bitcoin/pull/33507/files#r2389659427
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659844)
This test case should be deleted as this is not the expected behavior, see https://github.com/bitcoin/bitcoin/pull/33507/files#r2389659427
💬 andrewtoth commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3349669141)
Have you seen https://github.com/bitcoin/bitcoin/pull/29415? Is there a reason you would want this if we had private broadcast?
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3349669141)
Have you seen https://github.com/bitcoin/bitcoin/pull/29415? Is there a reason you would want this if we had private broadcast?
💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389662377)
Precompiled regexes are optimized to NFAs, but this one doesn't even backtrack so it's likely really fast and very easy to read.
But looking at https://github.com/catchorg/Catch2/blob/devel/src/catch2/internal/catch_debugger.cpp#L95 it seems to me that we don't even need to parse it, just check if it starts with zero or not, maybe something like:
```C++
for (std::string line; !attached && std::getline(status, line);) {
if (line.starts_with("TracerPid:")) {
attached = line.find
...
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389662377)
Precompiled regexes are optimized to NFAs, but this one doesn't even backtrack so it's likely really fast and very easy to read.
But looking at https://github.com/catchorg/Catch2/blob/devel/src/catch2/internal/catch_debugger.cpp#L95 it seems to me that we don't even need to parse it, just check if it starts with zero or not, maybe something like:
```C++
for (std::string line; !attached && std::getline(status, line);) {
if (line.starts_with("TracerPid:")) {
attached = line.find
...
💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389746621)
nit: if you re-touch, same as `walletdb.cpp`, copyright dates needs to be updated...
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389746621)
nit: if you re-touch, same as `walletdb.cpp`, copyright dates needs to be updated...
🤔 pablomartin4btc reviewed a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3282443179)
Not sure about this, if there are no descriptor wallets during startup (no default, no one in settings), we would be logging the sqlite version but if the user had legacy wallets in settings, that would print a warning (`FAILED_LEGACY_DISABLED` -> `initWarning`) not showing the berkeley version of the DB instantiated (which code is not in `master` either but it doesn't look consistent). If we have a diff DB format in the future we'd need to change this LogDBInfo() (once someone finds it).
I'd
...
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3282443179)
Not sure about this, if there are no descriptor wallets during startup (no default, no one in settings), we would be logging the sqlite version but if the user had legacy wallets in settings, that would print a warning (`FAILED_LEGACY_DISABLED` -> `initWarning`) not showing the berkeley version of the DB instantiated (which code is not in `master` either but it doesn't look consistent). If we have a diff DB format in the future we'd need to change this LogDBInfo() (once someone finds it).
I'd
...
💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389748193)
nit: if you re-touch... please update the copyright (also to match its header <code>walletdb.h</code>)
<code>// Copyright (c) 2009-present The Bitcoin Core developers</code>
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389748193)
nit: if you re-touch... please update the copyright (also to match its header <code>walletdb.h</code>)
<code>// Copyright (c) 2009-present The Bitcoin Core developers</code>
💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2389936885)
Ah, I guess I could have written the comment clearer. I meant `Use the earliest Xcode requiring at least the version of macOS denoted in ...`. Otherwise, we'd unnecessarily limit our Xcode features to EOL versions of macOS.
Whether to pick the earliest minor version of Xcode or earliest major version of Xcode is not specified, but I think going with the earliest major version here seems fine, to be able to remove the fuzz.cpp workaround.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2389936885)
Ah, I guess I could have written the comment clearer. I meant `Use the earliest Xcode requiring at least the version of macOS denoted in ...`. Otherwise, we'd unnecessarily limit our Xcode features to EOL versions of macOS.
Whether to pick the earliest minor version of Xcode or earliest major version of Xcode is not specified, but I think going with the earliest major version here seems fine, to be able to remove the fuzz.cpp workaround.
💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3350089468)
> The minimum supported version in the release notes should only be changed with the along with changing the minimum supported version in depends.
Thx, done. However, I can't test the resulting depends change on macOS myself.
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3350089468)
> The minimum supported version in the release notes should only be changed with the along with changing the minimum supported version in depends.
Thx, done. However, I can't test the resulting depends change on macOS myself.
🤔 Eunovo reviewed a pull request: "test: Fix rpc_getblockstats for no-wallet environments"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3282780757)
I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3282780757)
I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
💬 Eunovo commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389993284)
https://github.com/bitcoin/bitcoin/pull/33184/commits/12c9c7d927091f3be72f4acd99c166a41c789ae9:
IIUC, `MiniWallet` doesn't require wallet rpc, so what caused the "-disablewallet" error?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389993284)
https://github.com/bitcoin/bitcoin/pull/33184/commits/12c9c7d927091f3be72f4acd99c166a41c789ae9:
IIUC, `MiniWallet` doesn't require wallet rpc, so what caused the "-disablewallet" error?
💬 Eunovo commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389995639)
https://github.com/bitcoin/bitcoin/pull/33184/commits/316cf41c25438d61f4e9568e20878fa374476a5a:
I see that the CI fails because this argument conflicts with the argument defined in `rpc_blockstats`.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389995639)
https://github.com/bitcoin/bitcoin/pull/33184/commits/316cf41c25438d61f4e9568e20878fa374476a5a:
I see that the CI fails because this argument conflicts with the argument defined in `rpc_blockstats`.
💬 maflcko commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390009870)
You'll also have to check the fee? Otherwise, this just seems like a duplicate of `sendmsgtopeer 0 "tx" "$txhex"`?
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390009870)
You'll also have to check the fee? Otherwise, this just seems like a duplicate of `sendmsgtopeer 0 "tx" "$txhex"`?
💬 maflcko commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3350177873)
> I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
I guess it depends on how long it takes to generate, but if it is just a few seconds, it seems easier not to cache.
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3350177873)
> I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
I guess it depends on how long it takes to generate, but if it is just a few seconds, it seems easier not to cache.