💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1274629530)
Fixed, thanks!
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1274629530)
Fixed, thanks!
🚀 fanquake merged a pull request: "test: create wallet specific for test_locked_wallet case"
(https://github.com/bitcoin/bitcoin/pull/28139)
(https://github.com/bitcoin/bitcoin/pull/28139)
💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1651340516)
ACK 1e09c265a9598df3cc39336e3bcccb993dacf3d8
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1651340516)
ACK 1e09c265a9598df3cc39336e3bcccb993dacf3d8
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274662192)
Yeah, this is very wrong :(
Before a `RPC_PARSE_ERROR` was raised if the argument is the wrong type, and a `RPC_MISC_ERROR` if the sighash type could not be parsed.
Now a `RPC_PARSE_ERROR` is still raised if the argument is the wrong type, and a `RPC_INVALID_PARAMETER` if the sighash type could not be parsed.
Will fix.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274662192)
Yeah, this is very wrong :(
Before a `RPC_PARSE_ERROR` was raised if the argument is the wrong type, and a `RPC_MISC_ERROR` if the sighash type could not be parsed.
Now a `RPC_PARSE_ERROR` is still raised if the argument is the wrong type, and a `RPC_INVALID_PARAMETER` if the sighash type could not be parsed.
Will fix.
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274663777)
Do you have a suggestion for how to handle the fuzz test then? Could just remove this line again and re-add the `runtime_error` catch in the test.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274663777)
Do you have a suggestion for how to handle the fuzz test then? Could just remove this line again and re-add the `runtime_error` catch in the test.
💬 MarcoFalke commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1274667131)
If the options is used to speedup tx relay / mempool sync, it should be named appropriately. For example `self.noban_tx_relay`. Also, the option shouldn't be set for tests that previously didn't have it set. Otherwise they are testing something else. (`noban` has other effects than just immediate trickle)
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1274667131)
If the options is used to speedup tx relay / mempool sync, it should be named appropriately. For example `self.noban_tx_relay`. Also, the option shouldn't be set for tests that previously didn't have it set. Otherwise they are testing something else. (`noban` has other effects than just immediate trickle)
💬 TheCharlatan commented on pull request "util: Remove DirIsWritable, GetUniquePath":
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1651360743)
Nice, Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1651360743)
Nice, Concept ACK.
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274669797)
> Could just remove this line again and re-add the runtime_error catch in the test.
yes
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274669797)
> Could just remove this line again and re-add the runtime_error catch in the test.
yes
💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651390108)
I think before working on GitHub Actions CI, it should be clear how `GITHUB_TOKEN` behaves inside GitHub CI and whether it allows code pushes from the CI or from pull requests to the main branch.
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651390108)
I think before working on GitHub Actions CI, it should be clear how `GITHUB_TOKEN` behaves inside GitHub CI and whether it allows code pushes from the CI or from pull requests to the main branch.
📝 TheCharlatan opened a pull request: "refactor: Revert additional univalue check in ParseSighashString"
(https://github.com/bitcoin/bitcoin/pull/28162)
This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557).
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in th
...
(https://github.com/bitcoin/bitcoin/pull/28162)
This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557).
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in th
...
📝 brunoerg converted_to_draft a pull request: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153)
This PR adds target for `{Legacy}ScriptPubKeyMan`. I'm working on a descriptor one and will do it in a separate file. I tried to focus here on functions that we use directly and we may have in some unit tests.
(https://github.com/bitcoin/bitcoin/pull/28153)
This PR adds target for `{Legacy}ScriptPubKeyMan`. I'm working on a descriptor one and will do it in a separate file. I tried to focus here on functions that we use directly and we may have in some unit tests.
💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651432106)
> I think before working on GitHub Actions CI, it should be clear how `GITHUB_TOKEN` behaves inside GitHub CI and whether it allows code pushes from the CI or from pull requests to the main branch.
GitHub Action docs have a plenty of hints about that:
- https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
- https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651432106)
> I think before working on GitHub Actions CI, it should be clear how `GITHUB_TOKEN` behaves inside GitHub CI and whether it allows code pushes from the CI or from pull requests to the main branch.
GitHub Action docs have a plenty of hints about that:
- https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
- https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1274710779)
Yea, I just realized it. I'm changing it!
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1274710779)
Yea, I just realized it. I'm changing it!
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1274712377)
Apologies for missing this. It is an undocumented requirement that the functional usdt tests are run as root (not sure why). So this will disable them.
I think this should either be reverted, or the usdt requirement for root be dropped.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1274712377)
Apologies for missing this. It is an undocumented requirement that the functional usdt tests are run as root (not sure why). So this will disable them.
I think this should either be reverted, or the usdt requirement for root be dropped.
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274723895)
https://github.com/bitcoin/bitcoin/pull/28162
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274723895)
https://github.com/bitcoin/bitcoin/pull/28162
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274724027)
https://github.com/bitcoin/bitcoin/pull/28162
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1274724027)
https://github.com/bitcoin/bitcoin/pull/28162
💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651450188)
Ok, so I presume "Maximum access for pull requests from public forked repositories" applies and no further action is needed? Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651450188)
Ok, so I presume "Maximum access for pull requests from public forked repositories" applies and no further action is needed? Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1651454924)
> Concept ACK, but the description has some issues:
>
> > Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.
>
> It was always invalid.
Reworded it to say "even more clearly invalid"
> > Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for con
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1651454924)
> Concept ACK, but the description has some issues:
>
> > Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.
>
> It was always invalid.
Reworded it to say "even more clearly invalid"
> > Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for con
...
💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651455731)
> Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?
Good idea.
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651455731)
> Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?
Good idea.
💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274732379)
Agreed, makes more sense. Will move down if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274732379)
Agreed, makes more sense. Will move down if I have to retouch.