💬 maflcko commented on pull request "refactor: Remove workaround for resolved MSVC bug":
(https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2890515265)
If contributors here have a microsoft account, they may want to upvote the issue report, so that microsoft is aware that there is some interest in having it fixed.
(https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2890515265)
If contributors here have a microsoft account, they may want to upvote the issue report, so that microsoft is aware that there is some interest in having it fixed.
👍 theStack approved a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2850301319)
re-ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
(as per `$ git range-diff 2be93f7b...6ee32aaa`)
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2850301319)
re-ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
(as per `$ git range-diff 2be93f7b...6ee32aaa`)
✅ laanwj closed a pull request: "subprocess: replace `fs::directory_iterator` with `readdir`"
(https://github.com/bitcoin/bitcoin/pull/32529)
(https://github.com/bitcoin/bitcoin/pull/32529)
💬 laanwj commented on pull request "subprocess: replace `fs::directory_iterator` with `readdir`":
(https://github.com/bitcoin/bitcoin/pull/32529#issuecomment-2890559685)
This isn't necessary if it's worked around some other way https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890480867
(https://github.com/bitcoin/bitcoin/pull/32529#issuecomment-2890559685)
This isn't necessary if it's worked around some other way https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890480867
💬 crStiv commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2890561971)
> Seems to contradict the existence of `-rpcwhitelist`
@luke-jr now seems to be better, what are your thoughts?
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2890561971)
> Seems to contradict the existence of `-rpcwhitelist`
@luke-jr now seems to be better, what are your thoughts?
🤔 hebasto reviewed a pull request: "ci: Move DEBUG=1 to centos task"
(https://github.com/bitcoin/bitcoin/pull/32560#pullrequestreview-2850329125)
Changes look correct. Waiting for CI.
(https://github.com/bitcoin/bitcoin/pull/32560#pullrequestreview-2850329125)
Changes look correct. Waiting for CI.
👍 hebasto approved a pull request: "doc: Adjust stale MSVC bug url"
(https://github.com/bitcoin/bitcoin/pull/32561#pullrequestreview-2850334992)
ACK fa330a5e38a8e9937778eeb53f06390d943bc42c.
(https://github.com/bitcoin/bitcoin/pull/32561#pullrequestreview-2850334992)
ACK fa330a5e38a8e9937778eeb53f06390d943bc42c.
🚀 hebasto merged a pull request: "doc: Adjust stale MSVC bug url"
(https://github.com/bitcoin/bitcoin/pull/32561)
(https://github.com/bitcoin/bitcoin/pull/32561)
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890582680)
I'd say finding compiler bugs is a valid use-case of the CI, but this one seems to be fixed already, so there is probably not much value in bisecting the exact gcc commit, especially given that it would be extremely tedious without a minimal non-intermittent reproducer.
(The time would be better spent on reducing other compiler bugs, such as https://github.com/bitcoin/bitcoin/issues/32276#issuecomment-2807267578)
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890582680)
I'd say finding compiler bugs is a valid use-case of the CI, but this one seems to be fixed already, so there is probably not much value in bisecting the exact gcc commit, especially given that it would be extremely tedious without a minimal non-intermittent reproducer.
(The time would be better spent on reducing other compiler bugs, such as https://github.com/bitcoin/bitcoin/issues/32276#issuecomment-2807267578)
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2095447036)
Why `libtool`?
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2095447036)
Why `libtool`?
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2095449704)
I'd add `pkgconf`.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2095449704)
I'd add `pkgconf`.
🤔 janb84 reviewed a pull request: "wallet: Fix logging of wallet version"
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2850352360)
ACK https://github.com/bitcoin/bitcoin/commit/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Fixes regression, improves code readability, adds a test to check if the wallet file version is correctly logged.
- Code review ✅
- Build & run tests ✅
- Ran the added tests against master (fails as expected):
```
wallet_createwallet.py | ✖ Failed | 3 s
wallet_createwallet.py --usecli | ✖ Failed | 3 s
```
- looked into #26021
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2850352360)
ACK https://github.com/bitcoin/bitcoin/commit/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Fixes regression, improves code readability, adds a test to check if the wallet file version is correctly logged.
- Code review ✅
- Build & run tests ✅
- Ran the added tests against master (fails as expected):
```
wallet_createwallet.py | ✖ Failed | 3 s
wallet_createwallet.py --usecli | ✖ Failed | 3 s
```
- looked into #26021
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095459011)
`MAX_TX_LEGACY_SIGOPS`?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095459011)
`MAX_TX_LEGACY_SIGOPS`?
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095462982)
`GetSigOpCount(/*fAccurate=*/true)`
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095462982)
`GetSigOpCount(/*fAccurate=*/true)`
💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890611969)
Friendly ping @laanwj @davidgumberg @hodlinator @sipsorcery :)
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890611969)
Friendly ping @laanwj @davidgumberg @hodlinator @sipsorcery :)
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095472966)
Although there are no `break` or `continue` statements in the loop, it would feel a bit more robust to move this check below the loop.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095472966)
Although there are no `break` or `continue` statements in the loop, it would feel a bit more robust to move this check below the loop.
🤔 rkrux reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076)
Benchmarks at 29cf059e6e592f7f82b982493a043219cdfa5b40
An update to my previous comment: https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2848375525
I dug a bit more and realised that in order to have a fair assessment between master and the PR, the `AvailableCoins` benchmark changes of the last commit 29cf059e6e592f7f82b982493a043219cdfa5b40 must be present in master as well so that both the results come from the same benchmark cases. I gcp'ed the commit onto master, remove
...
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076)
Benchmarks at 29cf059e6e592f7f82b982493a043219cdfa5b40
An update to my previous comment: https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2848375525
I dug a bit more and realised that in order to have a fair assessment between master and the PR, the `AvailableCoins` benchmark changes of the last commit 29cf059e6e592f7f82b982493a043219cdfa5b40 must be present in master as well so that both the results come from the same benchmark cases. I gcp'ed the commit onto master, remove
...
💬 laanwj commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890660965)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2890660965)
Concept ACK
💬 janb84 commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2890663386)
Would move the remark of the -rpcwhitelist to the end of the section and add something about the -rpcwhitelistdefault function that is needed to set the default on no whitelist for all users. The added documentation is still strong imo 👍
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2890663386)
Would move the remark of the -rpcwhitelist to the end of the section and add something about the -rpcwhitelistdefault function that is needed to set the default on no whitelist for all users. The added documentation is still strong imo 👍
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095501048)
Could add: `The per-transaction sigop limit introduced in BIP54 does not cover the witness.`
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2095501048)
Could add: `The per-transaction sigop limit introduced in BIP54 does not cover the witness.`