🚀 fanquake merged a pull request: "test: Avoid intermittent issues due to async events in validationinterface_tests"
(https://github.com/bitcoin/bitcoin/pull/28150)
(https://github.com/bitcoin/bitcoin/pull/28150)
📝 MarcoFalke opened a pull request: "ci: Move ASan USDT to persistent_worker"
(https://github.com/bitcoin/bitcoin/pull/28161)
To run the USDT functional tests, the ASan task currently requires the container host to run the Ubuntu Lunar Linux kernel (or later). Cirrus CI is the only provider that allows to spin up full VMs, however they will start to charge for all tasks (See slightly related discussion in https://github.com/bitcoin/bitcoin/issues/28098).
Since it is cheaper and recommended by Cirrus CI to just run a persistent worker, do that.
(https://github.com/bitcoin/bitcoin/pull/28161)
To run the USDT functional tests, the ASan task currently requires the container host to run the Ubuntu Lunar Linux kernel (or later). Cirrus CI is the only provider that allows to spin up full VMs, however they will start to charge for all tasks (See slightly related discussion in https://github.com/bitcoin/bitcoin/issues/28098).
Since it is cheaper and recommended by Cirrus CI to just run a persistent worker, do that.
💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1651285875)
>the attacker needs to both learn and then spoof the protected IP range
I think a malicious ISP gets that trivially (at least in the pre-BIP324 world) by simply monitoring the traffic.
In theory, specifying "forceinbound=iprange;n_extra_slots" instead of eviction is probably a good idea, but I'm not sure how ugly the code will look like. I remember max_connections stuff is already sophisticated :(
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1651285875)
>the attacker needs to both learn and then spoof the protected IP range
I think a malicious ISP gets that trivially (at least in the pre-BIP324 world) by simply monitoring the traffic.
In theory, specifying "forceinbound=iprange;n_extra_slots" instead of eviction is probably a good idea, but I'm not sure how ugly the code will look like. I remember max_connections stuff is already sophisticated :(
💬 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?