💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959279697)
nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when `skip_if_no_wallet` already does it?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959279697)
nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when `skip_if_no_wallet` already does it?
💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959304120)
eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959304120)
eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959342651)
```diff
- info.prev_hash = tip->phashBlock;
+ info.prev_hash = tip->pprev->phashBlock;
```
The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.
Reference: `MakeBlockInfo()` here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18
The `pprev` presence check can be avoided because the test is using `TestChain100Setup` that has 100 blocks mined already lea
...
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959342651)
```diff
- info.prev_hash = tip->phashBlock;
+ info.prev_hash = tip->pprev->phashBlock;
```
The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.
Reference: `MakeBlockInfo()` here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18
The `pprev` presence check can be avoided because the test is using `TestChain100Setup` that has 100 blocks mined already lea
...
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959355949)
> // to by-pass the wallet birth time.
Can you explain this a bit more? I ran the test w/o it and it works.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959355949)
> // to by-pass the wallet birth time.
Can you explain this a bit more? I ran the test w/o it and it works.
💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2665081851)
> I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2665081851)
> I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0
It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?
👍 TheCharlatan approved a pull request: "cmake: add optional source files to bitcoin_crypto and crc32c directly"
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2622958345)
ACK 9cf746d6631739df9c9f80accd5812b319efcfec
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2622958345)
ACK 9cf746d6631739df9c9f80accd5812b319efcfec
👍 TheCharlatan approved a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623021656)
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623021656)
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
🤔 maflcko reviewed a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2622863309)
left a question
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2622863309)
left a question
💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105)
nit in https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105)
nit in https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?
💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055)
nit in e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055)
nit in e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?
💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959430288)
nit in https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: I'd say it would be clearer to not have `self.paths` at all, and also remove the need to somehow import and handle `Binaries` in individual tests.
It would be better to fully move all touched code of this pull into `TestNode` and only allow getting binaries through the `TestNode` interface. The reasons are:
* All functional tests revolve around nodes (and their datadirs) and it is hard to imagine a t
...
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959430288)
nit in https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: I'd say it would be clearer to not have `self.paths` at all, and also remove the need to somehow import and handle `Binaries` in individual tests.
It would be better to fully move all touched code of this pull into `TestNode` and only allow getting binaries through the `TestNode` interface. The reasons are:
* All functional tests revolve around nodes (and their datadirs) and it is hard to imagine a t
...
💬 maflcko commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665166672)
> I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
Does a missing target really get silently skipped? Locally I get the expected failure:
```
$ cmake --build ./bld-cmake/ -j 1 --target check-security-missing ; echo $?
gmake: *** No rule to make target 'check-security-missing'. Stop.
2
```
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665166672)
> I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
Does a missing target really get silently skipped? Locally I get the expected failure:
```
$ cmake --build ./bld-cmake/ -j 1 --target check-security-missing ; echo $?
gmake: *** No rule to make target 'check-security-missing'. Stop.
2
```
💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665169319)
> Does a missing target really get silently skipped? Locally I get the expected failure:
Did you pass -DENABLE_HARDENING=OFF
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665169319)
> Does a missing target really get silently skipped? Locally I get the expected failure:
Did you pass -DENABLE_HARDENING=OFF
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2665175404)
I was wondering how Homebrew fixes this, well apparently they just ad-hoc sign on your machine: https://github.com/orgs/Homebrew/discussions/4582#discussioncomment-6242807
Here's a random Rust project that does codesign and notarize, but doesn't staple: https://www.randomerrata.com/articles/2024/notarize/
Since macOS doesn't know if a binary is notarized, and it doesn't have the staple locally, it seems inevitable that it's going to call home.
How nice of Apple to say this:
> We have
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2665175404)
I was wondering how Homebrew fixes this, well apparently they just ad-hoc sign on your machine: https://github.com/orgs/Homebrew/discussions/4582#discussioncomment-6242807
Here's a random Rust project that does codesign and notarize, but doesn't staple: https://www.randomerrata.com/articles/2024/notarize/
Since macOS doesn't know if a binary is notarized, and it doesn't have the staple locally, it seems inevitable that it's going to call home.
How nice of Apple to say this:
> We have
...
💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665183445)
Actually, I don't understand what you've tested here. `test-security-missing` isn't a target, so that result is expected.
If you configure with `-DENABLE_HARDENING=OFF`, you'll get:
```bash
cmake --build build --target check-security
Built target check-security
```
but nothing will have actually been done, because the target is a dummy.
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665183445)
Actually, I don't understand what you've tested here. `test-security-missing` isn't a target, so that result is expected.
If you configure with `-DENABLE_HARDENING=OFF`, you'll get:
```bash
cmake --build build --target check-security
Built target check-security
```
but nothing will have actually been done, because the target is a dummy.
💬 maflcko commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665205096)
Sorry, I missed that a dummy target was added. Makes sense to remove the dummy target. Removing the condition seems fine as well.
lgtm ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665205096)
Sorry, I missed that a dummy target was added. Makes sense to remove the dummy target. Removing the condition seems fine as well.
lgtm ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
🚀 fanquake merged a pull request: "contrib: Add deterministic-fuzz-coverage"
(https://github.com/bitcoin/bitcoin/pull/31836)
(https://github.com/bitcoin/bitcoin/pull/31836)
⚠️ hodlinator opened an issue: "test: feature_assumeutxo.py Windows timeout"
(https://github.com/bitcoin/bitcoin/issues/31894)
Ran into a CI timeout in *feature_assumeutxo.py* on Windows: https://github.com/hodlinator/bitcoin/actions/runs/13371466603/job/37340702068#step:12:123
In the combined log we see the following, "Restarting node to stop at height" is [referring to node 1](https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/test/functional/feature_assumeutxo.py#L586)):
```
test ...T14:10:53.420000Z TestFramework (INFO): Restarting node to stop at height 359
test ...T14:10:53.420000Z
...
(https://github.com/bitcoin/bitcoin/issues/31894)
Ran into a CI timeout in *feature_assumeutxo.py* on Windows: https://github.com/hodlinator/bitcoin/actions/runs/13371466603/job/37340702068#step:12:123
In the combined log we see the following, "Restarting node to stop at height" is [referring to node 1](https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/test/functional/feature_assumeutxo.py#L586)):
```
test ...T14:10:53.420000Z TestFramework (INFO): Restarting node to stop at height 359
test ...T14:10:53.420000Z
...
💬 hodlinator commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665304532)
@pinheadmz since you are working on the HTTP rewrite... have you noticed anything fishy regarding whether the stop-RPC will actually always get the response sent off back to the client or not?
Current stop-RPC implementation implies it should always work:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/src/rpc/server.cpp#L170-L172
But it's from 2015 and the log above makes me very suspicious.
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665304532)
@pinheadmz since you are working on the HTTP rewrite... have you noticed anything fishy regarding whether the stop-RPC will actually always get the response sent off back to the client or not?
Current stop-RPC implementation implies it should always work:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/src/rpc/server.cpp#L170-L172
But it's from 2015 and the log above makes me very suspicious.
💬 maflcko commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665368484)
It looks like the scheduler thread never exited? It may be related to the comment:
```
// After everything has been shut down, but before things get flushed, stop the
// the scheduler. After this point, SyncWithValidationInterfaceQueue() should not be called anymore
// as this would prevent the shutdown from completing.
if (node.scheduler) node.scheduler->stop();
```
However, I don't see which thread could have called `SyncWithValidationInterfaceQueue` for it to block.
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665368484)
It looks like the scheduler thread never exited? It may be related to the comment:
```
// After everything has been shut down, but before things get flushed, stop the
// the scheduler. After this point, SyncWithValidationInterfaceQueue() should not be called anymore
// as this would prevent the shutdown from completing.
if (node.scheduler) node.scheduler->stop();
```
However, I don't see which thread could have called `SyncWithValidationInterfaceQueue` for it to block.