💬 janb84 commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32720#discussion_r2142415579)
```suggestion
- With `PascalCase`, Acronyms must be treated as words. To minimize code churn, legacy identifiers that do not comply with this rule will remain unchanged and will only be renamed when the surrounding code is modified. Hence:
```
NIT: Do not tell what NOT to do but explain what to do. This suggestion explains (imho) what the intention is, treating acronyms as words, and therefor the PascalCase can easily be applied.
(https://github.com/bitcoin/bitcoin/pull/32720#discussion_r2142415579)
```suggestion
- With `PascalCase`, Acronyms must be treated as words. To minimize code churn, legacy identifiers that do not comply with this rule will remain unchanged and will only be renamed when the surrounding code is modified. Hence:
```
NIT: Do not tell what NOT to do but explain what to do. This suggestion explains (imho) what the intention is, treating acronyms as words, and therefor the PascalCase can easily be applied.
💬 sipa commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-2966248213)
-0, I don't see why this can't be left to the author, given how rare it is.
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-2966248213)
-0, I don't see why this can't be left to the author, given how rare it is.
💬 l0rinc commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142452856)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142452856)
Done, thanks
💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2142472335)
Does this argument `include_watchonly` not require the `DEPRECATED` flag now in the RPCResult description?
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2142472335)
Does this argument `include_watchonly` not require the `DEPRECATED` flag now in the RPCResult description?
💬 fanquake commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2966291005)
Looks like this is getting pulled in by `python-scikit-build-core` -> `"/gnu/store/8a28mn0n9bd0nwq5vd6d1lfhr1j27cbk-rust-ring-0.17.8.tar.gz.drv"` -> `"/gnu/store/wbfmclxgnmlqbin55ipb0x05i1yapiy5-go-1.21.13.drv"` -> `"/gnu/store/wb36d09msn72aamvj8hj4y55551rqilz-go-1.17.13.drv"` -> `"/gnu/store/mjvb787fwd5bb6m2z02hhy30n5jzd9gw-go-1.4-bootstrap-20171003.drv"`
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2966291005)
Looks like this is getting pulled in by `python-scikit-build-core` -> `"/gnu/store/8a28mn0n9bd0nwq5vd6d1lfhr1j27cbk-rust-ring-0.17.8.tar.gz.drv"` -> `"/gnu/store/wbfmclxgnmlqbin55ipb0x05i1yapiy5-go-1.21.13.drv"` -> `"/gnu/store/wb36d09msn72aamvj8hj4y55551rqilz-go-1.17.13.drv"` -> `"/gnu/store/mjvb787fwd5bb6m2z02hhy30n5jzd9gw-go-1.4-bootstrap-20171003.drv"`
💬 l0rinc commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142496127)
I'm not sure I fully understand the comment here. Are you saying that it's obvious that this is always true? if so, isn't that kinda' the role of an assert though :)?
If you think it's ridiculously trivial (though you had to look inside the methods to understand the relation, this assertion was meant to avoid doing that, since I'm replacing one with the other in the code, this should help reviewers understand why that's safe).
So here it's meant to document why subsequent code now relies o
...
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142496127)
I'm not sure I fully understand the comment here. Are you saying that it's obvious that this is always true? if so, isn't that kinda' the role of an assert though :)?
If you think it's ridiculously trivial (though you had to look inside the methods to understand the relation, this assertion was meant to avoid doing that, since I'm replacing one with the other in the code, this should help reviewers understand why that's safe).
So here it's meant to document why subsequent code now relies o
...
💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142521186)
It was only a problem on Windows. Made a note of that.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142521186)
It was only a problem on Windows. Made a note of that.
💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142522767)
Cleaner, taken, thanks!
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142522767)
Cleaner, taken, thanks!
💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142525731)
I was assuming UTF-8 support as yeah they always run with UTF-8 on but in theory this could be run alone without so I just changed the symbol. Thanks.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142525731)
I was assuming UTF-8 support as yeah they always run with UTF-8 on but in theory this could be run alone without so I just changed the symbol. Thanks.
💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142528061)
Good spot, shame it's more verbose but it's all easy code to understand so taken. Thanks for not only noticing but also giving an alternative.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142528061)
Good spot, shame it's more verbose but it's all easy code to understand so taken. Thanks for not only noticing but also giving an alternative.
💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142528523)
Done.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142528523)
Done.
💬 l0rinc commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2966406017)
I reran to verify (only on Mac, my Hetzner boxes are busy):
```bash
docker run --platform linux/s390x -it ubuntu:latest /bin/bash
```
and inside I did something like:
```bash
export DEBIAN_FRONTEND=noninteractive && apt update && apt install -y git build-essential cmake ccache pkg-config libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qtbase5-dev qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev python3 \
cd /mnt && git clone --depth=1 https://github.com/bitcoin/bitcoin.git
...
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2966406017)
I reran to verify (only on Mac, my Hetzner boxes are busy):
```bash
docker run --platform linux/s390x -it ubuntu:latest /bin/bash
```
and inside I did something like:
```bash
export DEBIAN_FRONTEND=noninteractive && apt update && apt install -y git build-essential cmake ccache pkg-config libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qtbase5-dev qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev python3 \
cd /mnt && git clone --depth=1 https://github.com/bitcoin/bitcoin.git
...
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2142554412)
It seems unavoidable to change ci in the same commit.
Previous iterations were changing `BUILD_CONFIG`, hence the formatting change. But I'll just drop them now.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2142554412)
It seems unavoidable to change ci in the same commit.
Previous iterations were changing `BUILD_CONFIG`, hence the formatting change. But I'll just drop them now.
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2142559690)
I did keep the Windows mention.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2142559690)
I did keep the Windows mention.
🤔 janb84 reviewed a pull request: "test: round difficulty and networkhashps"
(https://github.com/bitcoin/bitcoin/pull/32725#pullrequestreview-2920953165)
ACK 578ea3eedb285519762087a4b27d953d8f61667f
- Code review ✅
- compiled & tested ✅
The PR changes test to use a more reasonable rounded difficulty and networkhashps to remove false positives in testing.
(https://github.com/bitcoin/bitcoin/pull/32725#pullrequestreview-2920953165)
ACK 578ea3eedb285519762087a4b27d953d8f61667f
- Code review ✅
- compiled & tested ✅
The PR changes test to use a more reasonable rounded difficulty and networkhashps to remove false positives in testing.
💬 hodlinator commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142647316)
The pseudo-code here is roughly:
```diff
value = lookup(key);
if (!value) {
return;
}
+ assert(value belongs to key);
```
Grepped the codebase for calls to `LookupBlockIndex()` and couldn't find any where we `assert` that the returned index's hash matches what we looked it up by. So this a departure from the current level of defensiveness we use in the project. Heck, */src/index/coinstatsindex.cpp* and */src/kernel/coinstats.cpp* don't even check against a null return value.
...
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142647316)
The pseudo-code here is roughly:
```diff
value = lookup(key);
if (!value) {
return;
}
+ assert(value belongs to key);
```
Grepped the codebase for calls to `LookupBlockIndex()` and couldn't find any where we `assert` that the returned index's hash matches what we looked it up by. So this a departure from the current level of defensiveness we use in the project. Heck, */src/index/coinstatsindex.cpp* and */src/kernel/coinstats.cpp* don't even check against a null return value.
...
💬 TheCharlatan commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142663267)
Shouldn't checking `role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload()` be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the `BlockChecked` method.
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142663267)
Shouldn't checking `role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload()` be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the `BlockChecked` method.
👍 hodlinator approved a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2921071060)
ACK f4e259b31f5785190c4912975173c5025dfe349c
### Concept
Enables more functional tests along with script for fetching previous release binaries on Windows. Removes script support for building previous releases from source on all platforms, which is arguably tricky as it requires having the correct (versions of) dependencies available, and seems to not be well used.
### Changes since previous review
Incorporates feedback from https://github.com/bitcoin/bitcoin/pull/32219#pullrequestre
...
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2921071060)
ACK f4e259b31f5785190c4912975173c5025dfe349c
### Concept
Enables more functional tests along with script for fetching previous release binaries on Windows. Removes script support for building previous releases from source on all platforms, which is arguably tricky as it requires having the correct (versions of) dependencies available, and seems to not be well used.
### Changes since previous review
Incorporates feedback from https://github.com/bitcoin/bitcoin/pull/32219#pullrequestre
...
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142693766)
Thanks for taking my suggestion!
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142693766)
Thanks for taking my suggestion!
💬 l0rinc commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142779638)
Regardless of the implementation, what I was documenting here was the safety of changing one value to the other, without having to understand the details of why. If I touch again, I don't mind changing this, if other reviewer agree it's overly pedantic. In other cases, I have explicitly removed asserts like this in the next commit, the signal that this value was explicitly checked before.
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142779638)
Regardless of the implementation, what I was documenting here was the safety of changing one value to the other, without having to understand the details of why. If I touch again, I don't mind changing this, if other reviewer agree it's overly pedantic. In other cases, I have explicitly removed asserts like this in the next commit, the signal that this value was explicitly checked before.
📝 hodlinator opened a pull request: "wallet: Warn upon failing to scan directory"
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753