💬 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
💬 vasild commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-2182297379)
I re-reviewed this again and retain my ACK because IMO it is an improvement to `master` even without further changes because it reduces the scope where the mutex is held. I don't see the fact that it can go further as a blocker for this.
I would be happy to review changing `StdMutex m_cs` to `Mutex m_cs` here or in another PR.
> The PR description sounded like this was going to be the ultimate change
well, fwiw, the description contains "not introducing that change here".
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-2182297379)
I re-reviewed this again and retain my ACK because IMO it is an improvement to `master` even without further changes because it reduces the scope where the mutex is held. I don't see the fact that it can go further as a blocker for this.
I would be happy to review changing `StdMutex m_cs` to `Mutex m_cs` here or in another PR.
> The PR description sounded like this was going to be the ultimate change
well, fwiw, the description contains "not introducing that change here".
💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2182303482)
Rebased to fix conflicts.
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2182303482)
Rebased to fix conflicts.
💬 TheCharlatan commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2182315865)
Guix builds (aarch64):
```
7f69ccde68d336bee6141d2cbcec8046332dedc908ba428ac08299daeb6a5d41 guix-build-545bb6c96080/output/aarch64-linux-gnu/SHA256SUMS.part
e843d7b6003da9f6259da41019f328af337858a45e4197ee9c5050984dc9e255 guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu-debug.tar.gz
1fdc9afbf83b2e8ce19d8a2c5cd080d87172ad02c4ce41036962f1bf675a415a guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu.tar.gz
400f2e70a
...
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2182315865)
Guix builds (aarch64):
```
7f69ccde68d336bee6141d2cbcec8046332dedc908ba428ac08299daeb6a5d41 guix-build-545bb6c96080/output/aarch64-linux-gnu/SHA256SUMS.part
e843d7b6003da9f6259da41019f328af337858a45e4197ee9c5050984dc9e255 guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu-debug.tar.gz
1fdc9afbf83b2e8ce19d8a2c5cd080d87172ad02c4ce41036962f1bf675a415a guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu.tar.gz
400f2e70a
...
👍 MarnixCroes approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2132212545)
tested ACK 4a028cf54c0502bc9ba0804bf1ae413b20a007cb
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2132212545)
tested ACK 4a028cf54c0502bc9ba0804bf1ae413b20a007cb
💬 dergoegge commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182380008)
There are a few things that `--enable-fuzz` does that we definitely want it to do:
* As you said, this option disables all other binaries besides the `fuzz` binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (`--with-sanitizers=fuzzer` i.e. `-fsanitize=fuzzer` with clang), because the other binaries provide a main function.
* libFuzzer is not the only fuzzing engine. We would like to support as many engines as
...
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182380008)
There are a few things that `--enable-fuzz` does that we definitely want it to do:
* As you said, this option disables all other binaries besides the `fuzz` binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (`--with-sanitizers=fuzzer` i.e. `-fsanitize=fuzzer` with clang), because the other binaries provide a main function.
* libFuzzer is not the only fuzzing engine. We would like to support as many engines as
...
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182390844)
> > > Maybe wait for the CI to pass, then address the nits to test the created cache?
> >
> >
> > Oh, I guess only master creates the cache?
> > ```
> > if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> > ```
> >
> > So I guess this can be merged after addressing the nits.
>
> Maybe comment that line out in a temporary "NO MERGE" commit?
I've pushed a modified branch to my personal account. Here is a CI job: https://github.com/he
...
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182390844)
> > > Maybe wait for the CI to pass, then address the nits to test the created cache?
> >
> >
> > Oh, I guess only master creates the cache?
> > ```
> > if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> > ```
> >
> > So I guess this can be merged after addressing the nits.
>
> Maybe comment that line out in a temporary "NO MERGE" commit?
I've pushed a modified branch to my personal account. Here is a CI job: https://github.com/he
...