💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2668392827)
Rebased a604321c3e4bd50b52fa28e8567f6b068b2d2fb3 -> 251a55f2f0cc3cdfb7fa0015b76772586134cde3 ([kernelApi_24](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_24) -> [kernelApi_25](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_24..kernelApi_25))
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2668392827)
Rebased a604321c3e4bd50b52fa28e8567f6b068b2d2fb3 -> 251a55f2f0cc3cdfb7fa0015b76772586134cde3 ([kernelApi_24](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_24) -> [kernelApi_25](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_24..kernelApi_25))
💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1961532157)
> > It would be better to fully move all touched code of this pull into `TestNode` and only allow getting binaries through the `TestNode` interface.
>
> This doesn't make much sense to me. I wouldn't want tests to be using `self.nodes[0].binaries` if they aren't actually interacting with node 0, and when a node 0 object might not even exist. If tests just need to call some binaries, they should be able to use a standalone binaries object without creating a fake node or going through an unrela
...
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1961532157)
> > It would be better to fully move all touched code of this pull into `TestNode` and only allow getting binaries through the `TestNode` interface.
>
> This doesn't make much sense to me. I wouldn't want tests to be using `self.nodes[0].binaries` if they aren't actually interacting with node 0, and when a node 0 object might not even exist. If tests just need to call some binaries, they should be able to use a standalone binaries object without creating a fake node or going through an unrela
...
💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2668411603)
https://cirrus-ci.com/task/6088386139652096?logs=ci#L3797
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2668411603)
https://cirrus-ci.com/task/6088386139652096?logs=ci#L3797
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961542693)
Agreed, though both disappeared in my last push.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961542693)
Agreed, though both disappeared in my last push.
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961544741)
I went back to the old behavior.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961544741)
I went back to the old behavior.
💬 Zero-1729 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2668427141)
I agree with @sipa here, I think the scope and focus of this PR should remain the macOS external SSD exFAT issue which is well documented and easily testable (both the error and @willcl-ark's warning).
Once there is a similar level of replicability for the SD card or similar issue, there can be a separate follow up PR that addresses that.
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2668427141)
I agree with @sipa here, I think the scope and focus of this PR should remain the macOS external SSD exFAT issue which is well documented and easily testable (both the error and @willcl-ark's warning).
Once there is a similar level of replicability for the SD card or similar issue, there can be a separate follow up PR that addresses that.
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668432011)
> the current code first sets the tip and then starts the RPC, so this is mostly theoretical or about future changes that might swap the startup order.
Indeed, that's what it's supposed to do.
I ended up simplifying things a bit. The original `CHECK_NONFATAL(miner.getTip()` has been replaced with `JSONRPCError(RPC_IN_WARMUP`. So in the unlikely event there's no tip, the user gets a clear error. I also added an `Assume`, so we can fix things if it turns out `SetRPCWarmupFinished` was called
...
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668432011)
> the current code first sets the tip and then starts the RPC, so this is mostly theoretical or about future changes that might swap the startup order.
Indeed, that's what it's supposed to do.
I ended up simplifying things a bit. The original `CHECK_NONFATAL(miner.getTip()` has been replaced with `JSONRPCError(RPC_IN_WARMUP`. So in the unlikely event there's no tip, the user gets a clear error. I also added an `Assume`, so we can fix things if it turns out `SetRPCWarmupFinished` was called
...
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668483777)
I forgot that you can't use `Assume` in RPC code. Using `NONFATAL_UNREACHABLE` instead, though that's not as clear to the user.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668483777)
I forgot that you can't use `Assume` in RPC code. Using `NONFATAL_UNREACHABLE` instead, though that's not as clear to the user.
💬 purpleKarrot commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-2668512824)
There are two general problems with printing a build summary at configure time:
1. It is annoying visual clutter. The cmake configure log should contain status events and important warnings as well as errors. Detailed settings can be analyzed and modified in the cache editor such as `ccmake`. Currently, when using `ccmake`, this dreaded summary has to be dismissed with `e` after each configure run with `c`. This is because make treats the summary as a warning.
2. There is no way to know al
...
(https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-2668512824)
There are two general problems with printing a build summary at configure time:
1. It is annoying visual clutter. The cmake configure log should contain status events and important warnings as well as errors. Detailed settings can be analyzed and modified in the cache editor such as `ccmake`. Currently, when using `ccmake`, this dreaded summary has to be dismissed with `e` after each configure run with `c`. This is because make treats the summary as a warning.
2. There is no way to know al
...
📝 maflcko opened a pull request: "test: Assert unused port to debug intermittent issue 30030"
(https://github.com/bitcoin/bitcoin/pull/31903)
`p2p_i2p_ports.py` is intermittently failing in tests on Cirrus, GHA, and locally.
However, the reason is unclear.
Assert that the port is unused to possibly get more details for further actions.
(https://github.com/bitcoin/bitcoin/pull/31903)
`p2p_i2p_ports.py` is intermittently failing in tests on Cirrus, GHA, and locally.
However, the reason is unclear.
Assert that the port is unused to possibly get more details for further actions.
💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2668550971)
Not sure what to do here. Let's print the offending program name? See https://github.com/bitcoin/bitcoin/pull/31903
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2668550971)
Not sure what to do here. Let's print the offending program name? See https://github.com/bitcoin/bitcoin/pull/31903
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668567278)
Four CI machines consistently failing `rpc_blockchain.py` (either `--v1transport` or `--v2transport`) at:
```py
def assert_waitforheight(height, timeout=2):
assert_equal(
node.waitforblockheight(height=height, timeout=timeout)['height'],
current_height)
```
With "Remote end closed connection without response", but not much useful info otherwise. Can't reproduce locally yet.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668567278)
Four CI machines consistently failing `rpc_blockchain.py` (either `--v1transport` or `--v2transport`) at:
```py
def assert_waitforheight(height, timeout=2):
assert_equal(
node.waitforblockheight(height=height, timeout=timeout)['height'],
current_height)
```
With "Remote end closed connection without response", but not much useful info otherwise. Can't reproduce locally yet.
💬 maflcko commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062)
```suggestion
BlockRef current_block{*CHECK_NONFATAL(miner.getTip())};
// Abort if RPC came out of warmup too early
```
There is no need for optional, if the value can never be nullopt
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062)
```suggestion
BlockRef current_block{*CHECK_NONFATAL(miner.getTip())};
// Abort if RPC came out of warmup too early
```
There is no need for optional, if the value can never be nullopt
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961635509)
I think "DriveLetter:" is more confusing than "C:". This is an example, not some kind of catch-all pattern.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961635509)
I think "DriveLetter:" is more confusing than "C:". This is an example, not some kind of catch-all pattern.
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961638926)
(I'm fine with the "path\\to\\walletname"-part though).
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961638926)
(I'm fine with the "path\\to\\walletname"-part though).
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668606129)
Replaced `NONFATAL_UNREACHABLE` with `CHECK_NONFATAL` again by avoiding the intermediate `std::optional`, see https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062
Rebased and pushed again, though I haven't figured out the CI issue.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668606129)
Replaced `NONFATAL_UNREACHABLE` with `CHECK_NONFATAL` again by avoiding the intermediate `std::optional`, see https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062
Rebased and pushed again, though I haven't figured out the CI issue.
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668649415)
> Remote end closed connection without response
That probably means `bitcoind` crashed.
> node0 stderr ... optional ... Assertion 'this->_M_is_engaged()' failed.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668649415)
> Remote end closed connection without response
That probably means `bitcoind` crashed.
> node0 stderr ... optional ... Assertion 'this->_M_is_engaged()' failed.
✅ maflcko closed a pull request: "test: Assert unused port to debug intermittent issue 30030"
(https://github.com/bitcoin/bitcoin/pull/31903)
(https://github.com/bitcoin/bitcoin/pull/31903)
💬 maflcko commented on pull request "test: Assert unused port to debug intermittent issue 30030":
(https://github.com/bitcoin/bitcoin/pull/31903#issuecomment-2668650948)
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
[08:12:30.628] python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
[08:12:30.628] bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
(https://github.com/bitcoin/bitcoin/pull/31903#issuecomment-2668650948)
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
[08:12:30.628] python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
[08:12:30.628] bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
📝 l0rinc opened a pull request: "refactor: modernize outdated trait patterns using helper aliases (C++14)"
(https://github.com/bitcoin/bitcoin/pull/31904)
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-o
...
(https://github.com/bitcoin/bitcoin/pull/31904)
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-o
...