💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648456362)
Not sure what you mean. This is just a moved section to explain the three possible and required labels.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648456362)
Not sure what you mean. This is just a moved section to explain the three possible and required labels.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182061790)
ACK 66b9740011222002ae9026e32e03a117628daae1
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182061790)
ACK 66b9740011222002ae9026e32e03a117628daae1
💬 maflcko commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2182063190)
https://cirrus-ci.com/task/6263983722725376?logs=ci#L3388
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2182063190)
https://cirrus-ci.com/task/6263983722725376?logs=ci#L3388
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182067595)
> when enabled: engage the fuzzing harness (compile with `-fsanitize=fuzzer`), force compiling `src/test/fuzz/fuzz` (if it is even possible to disable it via another option), force disable all other targets
More than one sanitizer exists. For example, the CI system uses more than one sanitizer. So afterwards you'd have two redundant and confusing ways to set sanitizers. Not sure that is an improvement.
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182067595)
> when enabled: engage the fuzzing harness (compile with `-fsanitize=fuzzer`), force compiling `src/test/fuzz/fuzz` (if it is even possible to disable it via another option), force disable all other targets
More than one sanitizer exists. For example, the CI system uses more than one sanitizer. So afterwards you'd have two redundant and confusing ways to set sanitizers. Not sure that is an improvement.
💬 levantah commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182070025)
I would just like to note that @fjahr is counted as NACK by mistake (read fully his [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978) which is just mentioning that the other he is replying to can "give a NACK").
Tested ACK as I run fullrbf nodes aver since the configuration option appeared and this change just chenges the default.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182070025)
I would just like to note that @fjahr is counted as NACK by mistake (read fully his [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978) which is just mentioning that the other he is replying to can "give a NACK").
Tested ACK as I run fullrbf nodes aver since the configuration option appeared and this change just chenges the default.
💬 levantah commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182071121)
See also my previous comment is counted wrong.
Tested ACK
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182071121)
See also my previous comment is counted wrong.
Tested ACK
💬 maflcko commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2182092423)
There is no vendor lock-in, because switching back can be done trivially by calling `git revert commit_id` by providing the commit_id that switched the task. E.g. d97ddbe797f5b8b3bca0ee71b692e542b8990195.
The CI system is written exactly with that in mind: Not care about the outer host and be possible to run anywhere. No one is holding anyone back to spin up the CI anywhere they want. All they need to get is the source code at the commit id they want to test and a way to report back the CI lo
...
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2182092423)
There is no vendor lock-in, because switching back can be done trivially by calling `git revert commit_id` by providing the commit_id that switched the task. E.g. d97ddbe797f5b8b3bca0ee71b692e542b8990195.
The CI system is written exactly with that in mind: Not care about the outer host and be possible to run anywhere. No one is holding anyone back to spin up the CI anywhere they want. All they need to get is the source code at the commit id they want to test and a way to report back the CI lo
...
❤1
💬 Sjors commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648503373)
Ok, changed to "works" and added a link that explains what this does.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648503373)
Ok, changed to "works" and added a link that explains what this does.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182135918)
ACK c67f215ea5fd57cd05e5346b8cd292dd879303ff
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182135918)
ACK c67f215ea5fd57cd05e5346b8cd292dd879303ff
💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1648509473)
Thanks, I'll change this if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1648509473)
Thanks, I'll change this if I have to retouch.
💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182144557)
utACK 4ecbbd9b7fa6f30e1d297cd26b112d3148744f58
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182144557)
utACK 4ecbbd9b7fa6f30e1d297cd26b112d3148744f58
💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182161395)
Feel free to propose better solutions, the one in "Describe the solution you'd like" in OP is just an idea.
`--with-sanitizers` can be used to enable additional sanitizers. Why would that be confusing? Because when the fuzzing is enabled, then `-fsanitize=fuzzer` would be used for compilation and then it is redundant if one does `--with-sanitizers=fuzzer` in addition? That would be harmless and produce the expected results. Much like if one uses `--enable-werror` and in addition sets `CXXFLAG
...
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182161395)
Feel free to propose better solutions, the one in "Describe the solution you'd like" in OP is just an idea.
`--with-sanitizers` can be used to enable additional sanitizers. Why would that be confusing? Because when the fuzzing is enabled, then `-fsanitize=fuzzer` would be used for compilation and then it is redundant if one does `--with-sanitizers=fuzzer` in addition? That would be harmless and produce the expected results. Much like if one uses `--enable-werror` and in addition sets `CXXFLAG
...
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2182172611)
I think this can be closed for now. It seems too controversial for a simple test change?
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2182172611)
I think this can be closed for now. It seems too controversial for a simple test change?
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182182086)
> That would be harmless and produce the expected results.
No objection if a duplicate `-fsanitize=fuzzer` is harmless and works in all build settings.
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182182086)
> That would be harmless and produce the expected results.
No objection if a duplicate `-fsanitize=fuzzer` is harmless and works in all build settings.
👍 vasild approved a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2132036625)
ACK 7d3662fbe35032178c5a5e27e73c592268f6e41b
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2132036625)
ACK 7d3662fbe35032178c5a5e27e73c592268f6e41b
💬 vasild commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1648588599)
Good catch! The implicit cast to `bool` of `CThreadInterrupt` makes it easy to get such mistakes if the variable is a pointer to `CThreadInterrupt` or `std::optional<CThreadInterrupt>`. If `CThreadInterrupt` had an explicit method like `IsInterrupted()` then it would be more clear to call that one, but unfortunately it does not have such a method.
To be explicit we can do `if (m_interrupt->operator bool()) {` :laughing: :robot: :rofl:
Or, more seriously, because `m_interrupt` can never be
...
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1648588599)
Good catch! The implicit cast to `bool` of `CThreadInterrupt` makes it easy to get such mistakes if the variable is a pointer to `CThreadInterrupt` or `std::optional<CThreadInterrupt>`. If `CThreadInterrupt` had an explicit method like `IsInterrupted()` then it would be more clear to call that one, but unfortunately it does not have such a method.
To be explicit we can do `if (m_interrupt->operator bool()) {` :laughing: :robot: :rofl:
Or, more seriously, because `m_interrupt` can never be
...
💬 maflcko commented on pull request "refactor: remove extraneous lock annotations from function definitions":
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2182247885)
ACK 5729dbbb7424d02c5e5bc4f2eb340fdc1c0100b4 🦋
<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: ACK 5729dbbb7424d02c5e5bc4f2eb
...
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2182247885)
ACK 5729dbbb7424d02c5e5bc4f2eb340fdc1c0100b4 🦋
<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: ACK 5729dbbb7424d02c5e5bc4f2eb
...
💬 fjahr commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182266101)
> I would just like to note that @fjahr is counted as NACK by mistake (read fully his [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978) which is just mentioning that the other he is replying to can "give a NACK").
I didn't notice, I gave @DrahtBot a 👎 so it should be removed.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182266101)
> I would just like to note that @fjahr is counted as NACK by mistake (read fully his [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978) which is just mentioning that the other he is replying to can "give a NACK").
I didn't notice, I gave @DrahtBot a 👎 so it should be removed.
💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1648625983)
fixed
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1648625983)
fixed
💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2182290089)
Just fixed typo noticed by @mzumsande : https://github.com/bitcoin/bitcoin/compare/750f8b50a749e577fc11f2f9f79e06cdd29e66f5..2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2182290089)
Just fixed typo noticed by @mzumsande : https://github.com/bitcoin/bitcoin/compare/750f8b50a749e577fc11f2f9f79e06cdd29e66f5..2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28