💬 instagibbs commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1559496350)
don't think we need to deprecate testnet3

💬 ismaelsadeeq commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273842309)
> I haven't tried to compile that suggestion, but it'll probably fail `-Wsign-compare`.

Indeed compiling this emits `-Wsign-compare` warning

<details>
<summary>warning</summary>

```terminal

Making all in src
CXX libbitcoin_node_a-validation.o
validation.cpp:5664:81: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]
if (!maybe_assumeutxo_data.has_value() || maybe_assumeutxo_data->height != base_blockheigh
...

⚠️ Quite-my-Tempo opened an issue: ""system_tests/run_command" unit test fails"
(https://github.com/bitcoin/bitcoin/issues/30601)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I'm building v26.0 on Ubuntu 22.04 and Debian 12. The build succeeds without errors, but i got one failing unit test:
`test/system_tests.cpp(88): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
`

### Expected behaviour

all tests should pass

### Steps to reproduce

after building the binary just run "make check"

### Relevant log output


...

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707385731)
> i just default to test against mainnet when possible.

I do the opposite :sweat_smile: , so that a corrupt test is less likely to corrupt a developers (default) main datadir for their testing by accident, but no strong opinion. Just a style-nit in any case.

(Even if you disable the logging manually, I presume you'll have to select a network) But if it works without, then it is fine as well.

💬 maflcko commented on issue ""system_tests/run_command" unit test fails":
(https://github.com/bitcoin/bitcoin/issues/30601#issuecomment-2273857164)
See https://github.com/bitcoin/bitcoin/pull/28286, which is "up for grabs"

💬 maflcko commented on issue ""system_tests/run_command" unit test fails":
(https://github.com/bitcoin/bitcoin/issues/30601#issuecomment-2273863130)
As a temporary workaround, you can try `--disable-external-signer`, if you don't need it. Or set a different locale, for the tests.

💬 josibake commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2273867085)
> Not sure when I will have a chance to do these things, but I can definitely prioritize if there is interest in seeing this go forward

Definitely interest from my side, but not super urgent. For context, I'm going to be focusing on reviewing kernel and multiprocess PRs going forward, so I'm generally interested in anything related to those topics. I had noticed some discussion on other indexing related PRs regarding the approach in this PR vs other approaches, which is what led me here.

>
...

🤔 ismaelsadeeq reviewed a pull request: "Assumeutxo: Sanitize block height in metadata"
(https://github.com/bitcoin/bitcoin/pull/30516#pullrequestreview-2225657138)
Tested this on master

💬 ismaelsadeeq commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707385489)
In 51f197bd84916c494e9250926776b9efc3225100 "Assumeutxo: Sanitize block height in assumeutxo metadata "
This change was added in 98e119da35c4f7e74f0c423974d497e350c8e9fa and then removed in this commit. Why not just add it like this in the previous commit with the same error `msg` string, and then update the `msg` string in this commit?

💬 ismaelsadeeq commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707417109)
`AssumeutxoData::height` is `int`, IMO would be better for them to be the same type, so should we consider changing it also to `uint32_t`? this will avoid the issue https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273842309 during comparison or having to explicitly cast one.

👍 fanquake approved a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2225688307)
ACK a3cb309e7c31853f272bffaa65fb6ab0a7cc4083

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707417887)
I think we can just drop include checks like this (which don't produce defines used anywhere else in the codebase). I realise this kind of check was ported from Autotools, but I no-longer think it's necessary to do. Running a check to see if we can include `string.h`, just to then immediately compile something which includes `string.h`, seems like it should just be collapsed down to the compile. Looks like another one is `check_include_file_cxx(unistd.h HAVE_UNISTD_H)`.

💬 achow101 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2273898861)
ACK 6bfa26048dbafb91e9ca63ea8d3960271e798098

👍 brunoerg approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-2225714117)
utACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64

✅ Quite-my-Tempo closed an issue: ""system_tests/run_command" unit test fails"
(https://github.com/bitcoin/bitcoin/issues/30601)

💬 Quite-my-Tempo commented on issue ""system_tests/run_command" unit test fails":
(https://github.com/bitcoin/bitcoin/issues/30601#issuecomment-2273911989)
it worked! thanks!

🚀 achow101 merged a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775)

🤔 ryanofsky reviewed a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2225643495)
Thanks for the review!

Updated efdc120795e7c3ce914ee0677c3b69803ea5de1e -> cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 ([`pr/ipc-bind.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.2) -> [`pr/ipc-bind.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.2..pr/ipc-bind.3)) with suggestions

💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707371794)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253

> If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?

Sure added both things.

💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707372358)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253

> Nit: Could be declared `static`?

Sure, added static