💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426612726)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426612726)
Fixed
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426612919)
Taken
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426612919)
Taken
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426613203)
Yes, taken
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426613203)
Yes, taken
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426613876)
Thank you fixed in 94cd47f3460
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426613876)
Thank you fixed in 94cd47f3460
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426614021)
> In theory, no conversions are actually required, right? In theory UniValue could use u8string instead of string internally, and other parts of the codebase could too, so nothing would need to be converted?
Not sure if it makes sense to update the whole codebase from `std::string` to `std::u8string`, just so that UniValue can accept an `std::u8string`. I think it would be fine for UniValue to accept both string types, though, then one would have to be "converted"/copied to the other (I don't
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426614021)
> In theory, no conversions are actually required, right? In theory UniValue could use u8string instead of string internally, and other parts of the codebase could too, so nothing would need to be converted?
Not sure if it makes sense to update the whole codebase from `std::string` to `std::u8string`, just so that UniValue can accept an `std::u8string`. I think it would be fine for UniValue to accept both string types, though, then one would have to be "converted"/copied to the other (I don't
...
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426614030)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426614030)
Fixed
📝 hebasto opened a pull request: "ci: Set `HOMEBREW_NO_AUTO_UPDATE` to avoid unrelated failures"
(https://github.com/bitcoin/bitcoin/pull/29080)
CI builds, which use macOS image version `20231025.2`, suffer from Homebrew update failures.
This PR prevents such behavior.
(https://github.com/bitcoin/bitcoin/pull/29080)
CI builds, which use macOS image version `20231025.2`, suffer from Homebrew update failures.
This PR prevents such behavior.
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-1855704209)
Thank you for review @stickies-v
I Addressed your comments and taken suggestions.
Force pushed from https://github.com/bitcoin/bitcoin/commit/6ffb9569b7f1c603ee3bfa3b56ee312d975fd8c9 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 , [Compare diff](https://github.com/bitcoin/bitcoin/compare/6ffb9569b7f1c603ee3bfa3b56ee312d975fd8c9..e032462c44dba9b6961c1e18bd597d63c6afac02)
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-1855704209)
Thank you for review @stickies-v
I Addressed your comments and taken suggestions.
Force pushed from https://github.com/bitcoin/bitcoin/commit/6ffb9569b7f1c603ee3bfa3b56ee312d975fd8c9 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 , [Compare diff](https://github.com/bitcoin/bitcoin/compare/6ffb9569b7f1c603ee3bfa3b56ee312d975fd8c9..e032462c44dba9b6961c1e18bd597d63c6afac02)
💬 stickies-v commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855708025)
> but I think I can get over it.
[🎅 🎄 ](https://github.com/stickies-v/bitcoin/commit/fa5ba5a4252a0af16e3845ac7d0e68f9010d2a54)
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855708025)
> but I think I can get over it.
[🎅 🎄 ](https://github.com/stickies-v/bitcoin/commit/fa5ba5a4252a0af16e3845ac7d0e68f9010d2a54)
💬 ismaelsadeeq commented on pull request "ci: Set `HOMEBREW_NO_AUTO_UPDATE` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855712986)
utACK f3800ba21914dc91174c096291690847c1b47a92
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855712986)
utACK f3800ba21914dc91174c096291690847c1b47a92
🤔 Sjors reviewed a pull request: "test: verify spend from 999-of-999 taproot multisig wallet"
(https://github.com/bitcoin/bitcoin/pull/28212#pullrequestreview-1781604091)
Thanks for the rebase. I think the two CI failures are not related to your PR. You could do a fresh rebase to see if it's green now.
(https://github.com/bitcoin/bitcoin/pull/28212#pullrequestreview-1781604091)
Thanks for the rebase. I think the two CI failures are not related to your PR. You could do a fresh rebase to see if it's green now.
💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426605372)
Note to other reviewers: this `privmap` argument determines for which keys an xpriv is inserted into the descriptor.
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426605372)
Note to other reviewers: this `privmap` argument determines for which keys an xpriv is inserted into the descriptor.
💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426613810)
Unfortunately this leads to extreme CPU usage and a `sendtoaddress` RPC timeout even on my 2019 MacBook Pro.
I suggest sticking to a lower maximum for now, so we at least get this useful helper function, and leaving a note like:
```
// TODO: fix or work around performance bottleneck in `sendtoaddress`
// in order to test the maximum Taproot allowed 999-of-999.
```
My Macbook can easily handle 1-of-100. The 30 second timeout is hit somewhere between 1-of-750 and 1-of-1000. The CI mach
...
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426613810)
Unfortunately this leads to extreme CPU usage and a `sendtoaddress` RPC timeout even on my 2019 MacBook Pro.
I suggest sticking to a lower maximum for now, so we at least get this useful helper function, and leaving a note like:
```
// TODO: fix or work around performance bottleneck in `sendtoaddress`
// in order to test the maximum Taproot allowed 999-of-999.
```
My Macbook can easily handle 1-of-100. The 30 second timeout is hit somewhere between 1-of-750 and 1-of-1000. The CI mach
...
💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426591213)
Testing different keys makes sense, but let's keep this PR simple and leave it to a followup.
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426591213)
Testing different keys makes sense, but let's keep this PR simple and leave it to a followup.
💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426614316)
`...,XPRIV_{n}`
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1426614316)
`...,XPRIV_{n}`
💬 ismaelsadeeq commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#issuecomment-1855719536)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29056#issuecomment-1855719536)
Concept ACK
📝 hebasto converted_to_draft a pull request: "ci: Set `HOMEBREW_NO_AUTO_UPDATE` to avoid unrelated failures"
(https://github.com/bitcoin/bitcoin/pull/29080)
CI builds, which use macOS image version `20231025.2`, suffer from Homebrew update failures. For example, https://github.com/bitcoin/bitcoin/actions/runs/7199058794/job/19609891263.
This PR prevents such behavior.
(https://github.com/bitcoin/bitcoin/pull/29080)
CI builds, which use macOS image version `20231025.2`, suffer from Homebrew update failures. For example, https://github.com/bitcoin/bitcoin/actions/runs/7199058794/job/19609891263.
This PR prevents such behavior.
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855753635)
> > but I think I can get over it.
>
> [🎅 🎄 ](https://github.com/stickies-v/bitcoin/commit/fa5ba5a4252a0af16e3845ac7d0e68f9010d2a54)
It wasn't written by me, so I not setting the prefix. This makes it easier for me to (fuzzily) recall which commits were authored by me and whose weren't, by just looking at the prefix.
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855753635)
> > but I think I can get over it.
>
> [🎅 🎄 ](https://github.com/stickies-v/bitcoin/commit/fa5ba5a4252a0af16e3845ac7d0e68f9010d2a54)
It wasn't written by me, so I not setting the prefix. This makes it easier for me to (fuzzily) recall which commits were authored by me and whose weren't, by just looking at the prefix.
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855754578)
I pushed a commit, but it is not a scripted-diff.
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1855754578)
I pushed a commit, but it is not a scripted-diff.
💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855754967)
Sorry, `HOMEBREW_NO_AUTO_UPDATE` has been replaced with `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK`, which does fix the issue. See https://github.com/hebasto/bitcoin/actions/runs/7208587650/job/19637838995.
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855754967)
Sorry, `HOMEBREW_NO_AUTO_UPDATE` has been replaced with `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK`, which does fix the issue. See https://github.com/hebasto/bitcoin/actions/runs/7208587650/job/19637838995.