💬 hodlinator commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3569453173)
> > Would it be acceptable to emit an error and halt, saying that `-reindex` for cases where nodes don't have sufficient block data also requires decreasing `-minimumchainwork=` to whatever level we have in the block data? Could also explain that this is in order to keep logic consistent regardless of `-reindex`, and also say that running with lowered `-minimumchainwork` long-term is not advisable.
>
> It is possible to do this, but will this be considered a better solution than automatically
...
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3569453173)
> > Would it be acceptable to emit an error and halt, saying that `-reindex` for cases where nodes don't have sufficient block data also requires decreasing `-minimumchainwork=` to whatever level we have in the block data? Could also explain that this is in order to keep logic consistent regardless of `-reindex`, and also say that running with lowered `-minimumchainwork` long-term is not advisable.
>
> It is possible to do this, but will this be considered a better solution than automatically
...
📝 maflcko opened a pull request: "ci: Use latest Xcode that the minimum macOS version allows"
(https://github.com/bitcoin/bitcoin/pull/33932)
Changing the CI policy to use the *latest* Xcode (instead of the *earliest*), allowed by the Bitcoin Core minimum supported macOS version, makes sense: While this may require the developer or user to install a later security point-release on macOS, this should generally be fine and it is even expected that users run the latest supported security release of their operating system. Also, in practise, this often doesn't result in a visible change anyway: This specific change from Xcode 16.0 to 16.2
...
(https://github.com/bitcoin/bitcoin/pull/33932)
Changing the CI policy to use the *latest* Xcode (instead of the *earliest*), allowed by the Bitcoin Core minimum supported macOS version, makes sense: While this may require the developer or user to install a later security point-release on macOS, this should generally be fine and it is even expected that users run the latest supported security release of their operating system. Also, in practise, this often doesn't result in a visible change anyway: This specific change from Xcode 16.0 to 16.2
...
💬 maflcko commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2555155096)
> @fanquake / @maflcko - are there plans to bump to a more recent version of xcode to have proper C++20 support?
Yeah, it should be possible to bump to the latest version. Though, it may be best to first make it deterministic: https://github.com/bitcoin/bitcoin/pull/32009. Otherwise, it will be harder to re-create.
> For proper spaceship operator support we would need: https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes:
That is a bit odd, because
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2555155096)
> @fanquake / @maflcko - are there plans to bump to a more recent version of xcode to have proper C++20 support?
Yeah, it should be possible to bump to the latest version. Though, it may be best to first make it deterministic: https://github.com/bitcoin/bitcoin/pull/32009. Otherwise, it will be harder to re-create.
> For proper spaceship operator support we would need: https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes:
That is a bit odd, because
...
💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3569687541)
> This suggestion was meant for the case where we can't do headers sync due to being offline (using zero peers after a certain timeout or something like that, I'm not sure how to best check whether we are offline). I guess the error message could also point out that going online should resolve the situation.
I see. A message that indicates that the node is waiting for headers sync to finish reindex will be valuable. We probably don't need to check that we are offline at all.
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3569687541)
> This suggestion was meant for the case where we can't do headers sync due to being offline (using zero peers after a certain timeout or something like that, I'm not sure how to best check whether we are offline). I guess the error message could also point out that going online should resolve the situation.
I see. A message that indicates that the node is waiting for headers sync to finish reindex will be valuable. We probably don't need to check that we are offline at all.
💬 vasild commented on issue "Issues with new create wallet dialogue":
(https://github.com/bitcoin-core/gui/issues/151#issuecomment-3569746495)
> Checking `Disable Private Keys` checks `Make Blank Wallet` (and disables `Encrypt Wallet`). This is correct.
This is no more in the latest version (as of 17072f7005), I guess it has changed in the mean time:
Initial state:
field | state | is clickable
---|---|---
`Encrypt Wallet` | ☐ | yes
`Disable Private Keys` | ☐ | yes
`Make Blank Wallet` | ☐ | yes
When I click on `Disable Private Keys`:
field | state | is clickable
---|---|---
`Encrypt Wallet` | ☐ | no
`Disable Private Keys` | ☑ | yes
`
...
(https://github.com/bitcoin-core/gui/issues/151#issuecomment-3569746495)
> Checking `Disable Private Keys` checks `Make Blank Wallet` (and disables `Encrypt Wallet`). This is correct.
This is no more in the latest version (as of 17072f7005), I guess it has changed in the mean time:
Initial state:
field | state | is clickable
---|---|---
`Encrypt Wallet` | ☐ | yes
`Disable Private Keys` | ☐ | yes
`Make Blank Wallet` | ☐ | yes
When I click on `Disable Private Keys`:
field | state | is clickable
---|---|---
`Encrypt Wallet` | ☐ | no
`Disable Private Keys` | ☑ | yes
`
...
💬 maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2555503278)
// Also the wallet prevents creating transaction with base fee above maxtxfee. -> // Also the wallet prevents creating transactions with base fee above maxtxfee. [“creating transaction” is ungrammatical; plural or an article is needed for clarity]
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2555503278)
// Also the wallet prevents creating transaction with base fee above maxtxfee. -> // Also the wallet prevents creating transactions with base fee above maxtxfee. [“creating transaction” is ungrammatical; plural or an article is needed for clarity]
💬 Sjors commented on issue "should concurrent IPC requests directed to the same thread cause a crash?":
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3569919239)
> to be honest, we could have gotten away with only 2 by forcing `getHeader`, `getCoinbaseTx`, `getCoinbaseMerklePath` and waitNext to be done sequentially under the same server thread
I think you should bench this before deciding if it needs optimising and also check whether it's _actually_ faster and not e.g. slowing the node responses down with overhead.
If `getHeader`, `getCoinbase(Tx)` and `getCoinbaseMerklePath` really need to be called in parallel, then I think it would be better if the
...
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3569919239)
> to be honest, we could have gotten away with only 2 by forcing `getHeader`, `getCoinbaseTx`, `getCoinbaseMerklePath` and waitNext to be done sequentially under the same server thread
I think you should bench this before deciding if it needs optimising and also check whether it's _actually_ faster and not e.g. slowing the node responses down with overhead.
If `getHeader`, `getCoinbase(Tx)` and `getCoinbaseMerklePath` really need to be called in parallel, then I think it would be better if the
...
💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3569959628)
I'm planning to make similar plots for #33922 (without CPU and ideally with marks to indicate where blocks were found).
If you're measuring the process memory instead of only the template memory (which #33922 enables), you'll want to hold the mempool itself constant. E.g. by picking some value for `-maxmempool` and waiting for it to fill before starting the measurement. You also want to set `-dbcache` to its minimum, because that's also accuring
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3569959628)
I'm planning to make similar plots for #33922 (without CPU and ideally with marks to indicate where blocks were found).
If you're measuring the process memory instead of only the template memory (which #33922 enables), you'll want to hold the mempool itself constant. E.g. by picking some value for `-maxmempool` and waiting for it to fill before starting the measurement. You also want to set `-dbcache` to its minimum, because that's also accuring
🤔 rkrux reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3499650048)
ACK 6e37b3b4861733ca97ec5d27d0bf52d187b6a2c9
I do remember facing issues on Mac some time back that prompted me to create issue #33667.
Like mentioned in earlier couple comments, it'd be helpful to mention that Linux is recommended in the beginning "Quickstart guide" section.
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3499650048)
ACK 6e37b3b4861733ca97ec5d27d0bf52d187b6a2c9
I do remember facing issues on Mac some time back that prompted me to create issue #33667.
Like mentioned in earlier couple comments, it'd be helpful to mention that Linux is recommended in the beginning "Quickstart guide" section.
📝 rejected-l opened a pull request: "CI: migrate workflows to checkout v6"
(https://github.com/bitcoin/bitcoin/pull/33933)
Updates `actions/checkout` to v6 in CI workflows. No code changes.
https://github.com/actions/checkout/releases/tag/v6.0.0
(https://github.com/bitcoin/bitcoin/pull/33933)
Updates `actions/checkout` to v6 in CI workflows. No code changes.
https://github.com/actions/checkout/releases/tag/v6.0.0
💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3570105870)
> Delegating template eviction responsibility to the client can put us in a situation where they handle it poorly and cause us to OOM
Note that it's _already_ the clients responsibility, that's inherent to how multiprocess works.
In the scenario where they handle it poorly, we can use FIFO deletion. All `getMemoryLoad()` does is give clients an opportunity to handle it better. If they're fine with FIFO, then they never have to call this method.
> treat clients differently from our own c
...
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3570105870)
> Delegating template eviction responsibility to the client can put us in a situation where they handle it poorly and cause us to OOM
Note that it's _already_ the clients responsibility, that's inherent to how multiprocess works.
In the scenario where they handle it poorly, we can use FIFO deletion. All `getMemoryLoad()` does is give clients an opportunity to handle it better. If they're fine with FIFO, then they never have to call this method.
> treat clients differently from our own c
...
🤔 rkrux reviewed a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909#pullrequestreview-3499758912)
ACK d14d1b52b51c4e8e2388af7e54c854e8a133e088
I vaguely recall noticing this limit some time back. Makes sense to me that it is highlighted in the job name.
(https://github.com/bitcoin/bitcoin/pull/33909#pullrequestreview-3499758912)
ACK d14d1b52b51c4e8e2388af7e54c854e8a133e088
I vaguely recall noticing this limit some time back. Makes sense to me that it is highlighted in the job name.
💬 rkrux commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2555796819)
This or something similar?
```diff
- test-each-commit:
+ test-last-few-ancestor-commits:
```
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2555796819)
This or something similar?
```diff
- test-each-commit:
+ test-last-few-ancestor-commits:
```
✅ maflcko closed a pull request: "CI: migrate workflows to checkout v6"
(https://github.com/bitcoin/bitcoin/pull/33933)
(https://github.com/bitcoin/bitcoin/pull/33933)
💬 maflcko commented on pull request "CI: migrate workflows to checkout v6":
(https://github.com/bitcoin/bitcoin/pull/33933#issuecomment-3570216444)
thx, but subtrees need to be modified upstream, not here
(https://github.com/bitcoin/bitcoin/pull/33933#issuecomment-3570216444)
thx, but subtrees need to be modified upstream, not here
💬 maflcko commented on pull request "ci: Run GUI unit tests in cross-Windows task":
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3570225361)
Looks like the output is missing, but this works: https://github.com/bitcoin/bitcoin/actions/runs/19630865894/job/56211087066#step:8:206:
```
Run ./bin/test_bitcoin-qt.exe
Error: Process completed with exit code 1.
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3570225361)
Looks like the output is missing, but this works: https://github.com/bitcoin/bitcoin/actions/runs/19630865894/job/56211087066#step:8:206:
```
Run ./bin/test_bitcoin-qt.exe
Error: Process completed with exit code 1.
👍 vasild approved a pull request: "ci: Use latest Xcode that the minimum macOS version allows"
(https://github.com/bitcoin/bitcoin/pull/33932#pullrequestreview-3500009912)
ACK fa9537cde10120b12c96061cbc3f79a7680f9d64
(https://github.com/bitcoin/bitcoin/pull/33932#pullrequestreview-3500009912)
ACK fa9537cde10120b12c96061cbc3f79a7680f9d64
📝 wedjob0X opened a pull request: "Update GitHub Actions "
(https://github.com/bitcoin/bitcoin/pull/33934)
Updated checkout action to v6 for Node.js 24 support and enhanced credential security.
v6 - https://github.com/actions/checkout/releases/tag/v6.0.0
(https://github.com/bitcoin/bitcoin/pull/33934)
Updated checkout action to v6 for Node.js 24 support and enhanced credential security.
v6 - https://github.com/actions/checkout/releases/tag/v6.0.0
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556032057)
Here the failure was not about the `<=>` operator itself, but because `NodeClock::time_point` did not have such an operator provided by the standard library. That was only for macos-cross CI jobs, which use Clang 19.1.7. The native macos jobs succeeded. They use XCode 16.0 and Clang 16.0.0.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556032057)
Here the failure was not about the `<=>` operator itself, but because `NodeClock::time_point` did not have such an operator provided by the standard library. That was only for macos-cross CI jobs, which use Clang 19.1.7. The native macos jobs succeeded. They use XCode 16.0 and Clang 16.0.0.
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3570474430)
I pushed an [update](https://github.com/bitcoin/bitcoin/compare/858f49bcb3590b211f014e8d290008bb13f5b17f..5b05a9959f1633bfee78d9edb180c672b0640ab5). I also don't think we should implement a fallback to support a rare usecase for few individuals. I've run into a few occasions where I used older bitcoind and newer cli but it's when I misconfigure something accidentally and wasn't the behaviour I intended. I guess this is what most normal users would run into.
This is why I think we shouldn't im
...
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3570474430)
I pushed an [update](https://github.com/bitcoin/bitcoin/compare/858f49bcb3590b211f014e8d290008bb13f5b17f..5b05a9959f1633bfee78d9edb180c672b0640ab5). I also don't think we should implement a fallback to support a rare usecase for few individuals. I've run into a few occasions where I used older bitcoind and newer cli but it's when I misconfigure something accidentally and wasn't the behaviour I intended. I guess this is what most normal users would run into.
This is why I think we shouldn't im
...