💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138092334)
Good catch, fixed. It's eventually changed to `ToGenTxid(inv)` so I use `ToVariant()` here and remove that later on.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138092334)
Good catch, fixed. It's eventually changed to `ToGenTxid(inv)` so I use `ToVariant()` here and remove that later on.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138107221)
Fixed. I thought there was some reason I chose not to do this from the start, but I forgot.. so yeah I agree this is better.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138107221)
Fixed. I thought there was some reason I chose not to do this from the start, but I forgot.. so yeah I agree this is better.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138114543)
Thanks for reviewing. I don't think I can improve upon your suggested commit message, so hope you don't mind that I took this directly.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138114543)
Thanks for reviewing. I don't think I can improve upon your suggested commit message, so hope you don't mind that I took this directly.
💬 willcl-ark commented on pull request "depends: fix cmake compatibility error for freetype":
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2959586147)
Outside of being statically-linked or not, it would seem surprising to me to build with (a) newer version(s) from depends than is generally available on the platforms we support. If we for example used a new feature from the version in depends this would presumably break building on some (older/slower to update) platforms.
I'd always assumed we ~ (if not strictly) updated depends in line with which versions are available on our supported platforms.
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2959586147)
Outside of being statically-linked or not, it would seem surprising to me to build with (a) newer version(s) from depends than is generally available on the platforms we support. If we for example used a new feature from the version in depends this would presumably break building on some (older/slower to update) platforms.
I'd always assumed we ~ (if not strictly) updated depends in line with which versions are available on our supported platforms.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138142820)
I'm leaving the rest of the mempool for the next (and final) type safety PR. My goal for this PR is to only touch GenTxid instances. I think if I start changing things here, it cascades and I would have to address a lot more of the mempool.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2138142820)
I'm leaving the rest of the mempool for the next (and final) type safety PR. My goal for this PR is to only touch GenTxid instances. I think if I start changing things here, it cascades and I would have to address a lot more of the mempool.
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138077597)
The fuzz crash reported by @marcofleon in https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2539605981 occurred because we were comparing the simple chunking with the chunking read from the fuzzer.
However, in the block, `simple_chunking` might not be optimal, while chunking from `Linearize` is optimal. In such cases, the linearization provided by the fuzzer might be better than what is produced by `SimpleLinearize` in `simple_chunking`.
Can be reproduced by appending this diff
...
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138077597)
The fuzz crash reported by @marcofleon in https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2539605981 occurred because we were comparing the simple chunking with the chunking read from the fuzzer.
However, in the block, `simple_chunking` might not be optimal, while chunking from `Linearize` is optimal. In such cases, the linearization provided by the fuzzer might be better than what is produced by `SimpleLinearize` in `simple_chunking`.
Can be reproduced by appending this diff
...
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2137965569)
IIUC `FindCandidateSet` only adds connected sets and there is is comment in line 74-75 that explains that.
so even when not optimal this assertion will hold.
I've been running a version of this commit with the assertion above if optimal and no crash after a long time.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2137965569)
IIUC `FindCandidateSet` only adds connected sets and there is is comment in line 74-75 that explains that.
so even when not optimal this assertion will hold.
I've been running a version of this commit with the assertion above if optimal and no crash after a long time.
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138123670)
Should we also do more comparison when both are optimal, that the chunk matches?
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138123670)
Should we also do more comparison when both are optimal, that the chunk matches?
🤔 ismaelsadeeq reviewed a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2913685583)
reACK d2fc10b64505512a70bed05e7bb21d76c8f748cd
With some comments, even though some things added here are deleted in #32545
I think this is quite useful and should get in before SFL.
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2913685583)
reACK d2fc10b64505512a70bed05e7bb21d76c8f748cd
With some comments, even though some things added here are deleted in #32545
I think this is quite useful and should get in before SFL.
🤔 maflcko reviewed a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2913984991)
this touches ~500 lines, but removes ~200 redundant lines, which makes them hard to read/write/maintain. So concept ack.
lgtm ACK 04b91f865f3c2dd51df9d217afaaf88e1bcb0396 🐎
<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}"
RUTRmVTMeKV5npGrKx1
...
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2913984991)
this touches ~500 lines, but removes ~200 redundant lines, which makes them hard to read/write/maintain. So concept ack.
lgtm ACK 04b91f865f3c2dd51df9d217afaaf88e1bcb0396 🐎
<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}"
RUTRmVTMeKV5npGrKx1
...
💬 maflcko commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138143141)
nit in a1b941d0b48b7927a45ff40cc20902f835240898: Not sure it makes sense to change the code only to change it again in the next commit (cd0bbb4bbadf0d19eaa321a326967cf7dbf1c2df)
What about moving this after cd0bbb4bbadf0d19eaa321a326967cf7dbf1c2df?
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138143141)
nit in a1b941d0b48b7927a45ff40cc20902f835240898: Not sure it makes sense to change the code only to change it again in the next commit (cd0bbb4bbadf0d19eaa321a326967cf7dbf1c2df)
What about moving this after cd0bbb4bbadf0d19eaa321a326967cf7dbf1c2df?
💬 fanquake commented on pull request "depends: fix cmake compatibility error for freetype":
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2959624804)
> it would seem surprising to me to build with (a) newer version(s) from depends than is generally available on the platforms we support.
This is why everything non-GUI from depends is statically linked, so that the runtime versions, don't matter.
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2959624804)
> it would seem surprising to me to build with (a) newer version(s) from depends than is generally available on the platforms we support.
This is why everything non-GUI from depends is statically linked, so that the runtime versions, don't matter.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138186805)
Ok I must have been tripping yesterday... changed
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138186805)
Ok I must have been tripping yesterday... changed
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138187785)
Thanks! Squashed this in
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2138187785)
Thanks! Squashed this in
🤔 rkrux reviewed a pull request: "wallet, refactor: Remove Legacy wallet unused warnings and errors"
(https://github.com/bitcoin/bitcoin/pull/32481#pullrequestreview-2914057578)
Looking good based on a cursory glance, will take a deeper look soon.
Maybe mention a note in the PR description about wallet migration and how removing these checks don't affect that flow?
(https://github.com/bitcoin/bitcoin/pull/32481#pullrequestreview-2914057578)
Looking good based on a cursory glance, will take a deeper look soon.
Maybe mention a note in the PR description about wallet migration and how removing these checks don't affect that flow?
💬 rkrux commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138185014)
nit if retouched: extra blank line here
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138185014)
nit if retouched: extra blank line here
💬 fanquake commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2959689596)
@marcofleon you might be interested in reviewing here?
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2959689596)
@marcofleon you might be interested in reviewing here?
💬 maflcko commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138195930)
(aborted my review after this comment, so it is only partial right now, but I'll continue once there is a reply here)
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138195930)
(aborted my review after this comment, so it is only partial right now, but I'll continue once there is a reply here)
💬 fanquake commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2959710095)
Some other questions:
- How quickly does the data degrade? Im guessing not so fast that there could be any adverse effects within the supported lifetime of a binary?
- Is the expectation that we'd backport updates to any embedded data for every new (point) release?
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2959710095)
Some other questions:
- How quickly does the data degrade? Im guessing not so fast that there could be any adverse effects within the supported lifetime of a binary?
- Is the expectation that we'd backport updates to any embedded data for every new (point) release?
💬 Sjors commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-2959743190)
ACK 4f10a57671c19cacca630b2401e42a213aacff1b
I tested that this fixes #32691 on an aarch64 OpenBSD VM, and doesn't break anything on an x86 VM.
I didn't test FreeBSD.
While testing this I noticed the depends build doesn't bother creating qt. It's not obvious to me why that is and doesn't seem to be documented. The non-depends instructions for *BSD do include qt6.
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-2959743190)
ACK 4f10a57671c19cacca630b2401e42a213aacff1b
I tested that this fixes #32691 on an aarch64 OpenBSD VM, and doesn't break anything on an x86 VM.
I didn't test FreeBSD.
While testing this I noticed the depends build doesn't bother creating qt. It's not obvious to me why that is and doesn't seem to be documented. The non-depends instructions for *BSD do include qt6.