💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284457296)
Any reason to reorder, remove libevent and libtool?
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284457296)
Any reason to reorder, remove libevent and libtool?
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284458404)
nit: forgot to set the timeout? What is the default?
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284458404)
nit: forgot to set the timeout? What is the default?
💬 real-or-random commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665661274)
If we now have self-hosted runners for Cirrus, would it make sense (perhaps in the long-run) to make them also [self-hosted runners for GitHub Actions](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners)? This will remove Cirrus from the loop entirely, which, I think, is a good thing. Cirrus anyway acts only as a coordination layer between GitHub and the runner.
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665661274)
If we now have self-hosted runners for Cirrus, would it make sense (perhaps in the long-run) to make them also [self-hosted runners for GitHub Actions](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners)? This will remove Cirrus from the loop entirely, which, I think, is a good thing. Cirrus anyway acts only as a coordination layer between GitHub and the runner.
🤔 furszy reviewed a pull request: "test: Extend stale tip test"
(https://github.com/bitcoin/bitcoin/pull/18470#pullrequestreview-1562940257)
This test extension could play well with #28170. Are you planning to rebase it?
(https://github.com/bitcoin/bitcoin/pull/18470#pullrequestreview-1562940257)
This test extension could play well with #28170. Are you planning to rebase it?
💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284468527)
> My interpretation of these results is
I don't think you can use coverage as a metric when comparing two different code bases. The version that has higher coverage is also the version that has more code in the fuzz target, which is also counted toward "coverage".
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284468527)
> My interpretation of these results is
I don't think you can use coverage as a metric when comparing two different code bases. The version that has higher coverage is also the version that has more code in the fuzz target, which is also counted toward "coverage".
💬 iBeizsley commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665678418)
> I'm not going to war for this change but this strikes me as correct. I classified the OP_RETURN limit as the last remaining "paternalism" restriction in the codebase, since you can't even over-write how many datacarrier outputs are allowed via config.
If we are concerned about 'paternalisms', rather than changing default behaviour, which is itself making a decision for node runners, just allowing the overriding of the default by config is a better solution.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665678418)
> I'm not going to war for this change but this strikes me as correct. I classified the OP_RETURN limit as the last remaining "paternalism" restriction in the codebase, since you can't even over-write how many datacarrier outputs are allowed via config.
If we are concerned about 'paternalisms', rather than changing default behaviour, which is itself making a decision for node runners, just allowing the overriding of the default by config is a better solution.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284480443)
Trying to maximum the coverage in 100_000 runs for each target i managed to:
- achieve `2099` of coverage with the `ConsumeDeserializable`-based target using `-max_len=10000 -len_control=1 -mutate_depth=3` (in 305 seconds).
- achieve `2088` of coverage with the `ConsumeIntegral`-based target using -max_len=8000 -len_control=0 -use_value_profile=1 -mutate_depth=1` (in 252 seconds).
------
> I don't think you can use coverage as a metric
Oh, right.. I had overlooked this. However the di
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284480443)
Trying to maximum the coverage in 100_000 runs for each target i managed to:
- achieve `2099` of coverage with the `ConsumeDeserializable`-based target using `-max_len=10000 -len_control=1 -mutate_depth=3` (in 305 seconds).
- achieve `2088` of coverage with the `ConsumeIntegral`-based target using -max_len=8000 -len_control=0 -use_value_profile=1 -mutate_depth=1` (in 252 seconds).
------
> I don't think you can use coverage as a metric
Oh, right.. I had overlooked this. However the di
...
💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665708491)
> This will remove Cirrus from the loop entirely
Yes, that should work. The only difference seems to be that assigning labels is only possible manual (https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/using-labels-with-self-hosted-runners) and not from the command line that starts the runner.
But I guess in the short run it seems easier to stick to Cirrus for now, because the diff is a lot smaller (just replace `container:` in the yml with `persistent
...
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665708491)
> This will remove Cirrus from the loop entirely
Yes, that should work. The only difference seems to be that assigning labels is only possible manual (https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/using-labels-with-self-hosted-runners) and not from the command line that starts the runner.
But I guess in the short run it seems easier to stick to Cirrus for now, because the diff is a lot smaller (just replace `container:` in the yml with `persistent
...
💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284506824)
> I'm starting to lean toward reverting to this version.
There's also the possibility to add back the early return to the `ConsumeIntegral` one. (I haven't looked at the fuzz target to see if early return makes more or less sense)
> I feel like introducing a crash wouldn't be interesting given the low coverage of the target.
yeah, I guess it is hard to find a meaningful crash. I'd pick a line of code that is usually reached the "last" by coverage or is deeply nested.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284506824)
> I'm starting to lean toward reverting to this version.
There's also the possibility to add back the early return to the `ConsumeIntegral` one. (I haven't looked at the fuzz target to see if early return makes more or less sense)
> I feel like introducing a crash wouldn't be interesting given the low coverage of the target.
yeah, I guess it is hard to find a meaningful crash. I'd pick a line of code that is usually reached the "last" by coverage or is deeply nested.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284516335)
[[Usage limits](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits)](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits):
> Each job in a workflow can run for up to 6 hours of execution time.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284516335)
[[Usage limits](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits)](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits):
> Each job in a workflow can run for up to 6 hours of execution time.
💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284517464)
Yeah ok, that is also a possibility. Great.
Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284517464)
Yeah ok, that is also a possibility. Great.
Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284518653)
May be better to reduce it to two hours to catch timeout issues earlier? But no strong opinion, just a nit.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284518653)
May be better to reduce it to two hours to catch timeout issues earlier? But no strong opinion, just a nit.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522205)
Done.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522205)
Done.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522643)
Restored.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522643)
Restored.
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665737884)
re-ACK fe34609476b11cdd907c298f7300d424bf556e98 🐺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK fe34609476b11cdd907
...
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665737884)
re-ACK fe34609476b11cdd907c298f7300d424bf556e98 🐺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK fe34609476b11cdd907
...
💬 furszy commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1284531773)
In 2b67ea46:
Instead of adding the new function, could just delete the `RemoveBlockRequest()` line and directly call `BlockRequested()`. Failing with a `Already requested from this peer` if the call return false.
`BlockRequested()` only fails when the block is already in-flight for that peer (it does the same as your new `IsBlockRequestedFromPeer()` function).
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1284531773)
In 2b67ea46:
Instead of adding the new function, could just delete the `RemoveBlockRequest()` line and directly call `BlockRequested()`. Failing with a `Already requested from this peer` if the call return false.
`BlockRequested()` only fails when the block is already in-flight for that peer (it does the same as your new `IsBlockRequestedFromPeer()` function).
🤔 glozow reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1563072550)
ACK 1a118062fbc4ec8f645f4ec4298d869a869c3344
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1563072550)
ACK 1a118062fbc4ec8f645f4ec4298d869a869c3344
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1284547085)
> Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor
I don't think this is true. We should sort the transactions before we look up UTXOs.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1284547085)
> Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor
I don't think this is true. We should sort the transactions before we look up UTXOs.
💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239)
This fails locally:
```
node1 2023-08-02T03:08:04.791676Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
node1 2023-08-02T03:08:04.798619Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 3
node1 2023-08-02T03:08:04.807232Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 3
node1 2023-08-02T03:08:04.883158Z [http] [httpserver.cpp:255] [
...
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239)
This fails locally:
```
node1 2023-08-02T03:08:04.791676Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
node1 2023-08-02T03:08:04.798619Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 3
node1 2023-08-02T03:08:04.807232Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 3
node1 2023-08-02T03:08:04.883158Z [http] [httpserver.cpp:255] [
...
💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284568755)
https://drahtbot.space/temp_scratch/wallet_fundrawtransaction_251.tar.xz
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284568755)
https://drahtbot.space/temp_scratch/wallet_fundrawtransaction_251.tar.xz