💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164)
Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164)
Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963)
Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963)
Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885)
Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885)
Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
💬 dergoegge commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273782709)
> Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205?
Iirc, I pinged Antoine for both PRs on irc.
I also don't think the stability needs to be a blocker.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273782709)
> Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205?
Iirc, I pinged Antoine for both PRs on irc.
I also don't think the stability needs to be a blocker.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707318080)
I don't think there is any difference, i just default to test against mainnet when possible. On the other hand it sometimes makes sense to default to regtest instead (for instance in the context of a fuzz target touching validation it would enable the codepaths for all soft forks). So, yeah, :shrug:.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707318080)
I don't think there is any difference, i just default to test against mainnet when possible. On the other hand it sometimes makes sense to default to regtest instead (for instance in the context of a fuzz target touching validation it would enable the codepaths for all soft forks). So, yeah, :shrug:.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707322786)
Actually i think i can get rid of this now, and just disable logging like i do like in #29158.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707322786)
Actually i think i can get rid of this now, and just disable logging like i do like in #29158.
👍 Sjors approved a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2224313095)
tACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb
### Stalling fix
I tested (rebased on master) that without the second commit, when loading a testnet3 snapshot, and only having outbound peers, many peers keep downloading background sync blocks. A few newly connected peers do contribute to the tip.
With the second commit `synced_blocks` jumps to the snapshot height. Once the tip is reached (takes a while due to the loppocalypse and because the snapshot is a year old), and background sync ne
...
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2224313095)
tACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb
### Stalling fix
I tested (rebased on master) that without the second commit, when loading a testnet3 snapshot, and only having outbound peers, many peers keep downloading background sync blocks. A few newly connected peers do contribute to the tip.
With the second commit `synced_blocks` jumps to the snapshot height. Once the tip is reached (takes a while due to the loppocalypse and because the snapshot is a year old), and background sync ne
...
❤1
💬 Sjors commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706829412)
7a885518d57c6eb818ebef5fd04a575f324ee8b2: although this scenario is worthy of `LogWarning`, we have to be careful to avoid a log spam attack. So for now `LogDebug` is fine.
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706829412)
7a885518d57c6eb818ebef5fd04a575f324ee8b2: although this scenario is worthy of `LogWarning`, we have to be careful to avoid a log spam attack. So for now `LogDebug` is fine.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707325918)
Not sure why i have this include here, i'll just copy what you suggest.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707325918)
Not sure why i have this include here, i'll just copy what you suggest.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707326409)
For the following discussion, please see https://github.com/hebasto/bitcoin/pull/312.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707326409)
For the following discussion, please see https://github.com/hebasto/bitcoin/pull/312.
💬 instagibbs commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707354189)
If it's being considered for testnet3, I'd recommend a fairly long deprecation cycle, at least 2 releases.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707354189)
If it's being considered for testnet3, I'd recommend a fairly long deprecation cycle, at least 2 releases.
💬 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
(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
...
(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
...
(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.
(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"
(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.
(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.
>
...
(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
(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?
(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?