💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3580622759)
`7826148b12...ade7fcb6a0`: in the internals of the `PrivateBroadcast` class - drop the 2 indexes "by node" and "by priority" and use just one container. When a search by node or by priority is to be done, then full scan that single container. That is `O(n)` but as suggested above we will not store huge amount of entries. Searching by priority is actually finding the one with the highest priority.

So, organize the data in a map with a key=transaction and value=sending stats per node. The map c
...

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3580635290)
review ACK c0bfe72f6e1f63e05772eda959137b3d0bbbf6c3 🍨

<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: review ACK c0bfe72f6e1f
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564421629)
Removed the usage of `SaltedTxidHasher` altogether. Thanks!

💬 fanquake commented on pull request "depends: Boost 1.90.0":
(https://github.com/bitcoin/bitcoin/pull/33428#issuecomment-3580642994)
Pushed this to the `1.90.0.beta1`.

💬 fanquake commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3580645314)
cc @stickies-v

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564432684)
I switched to own-implemented operator `<=>`. "Resolve"ing this because in the latest version a `= default`ed operator `<=>` is not suitable.

💬 TheCharlatan commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3580676738)
I tend to agree with stickies-v here. I think when it comes to setting up these options structs we should support dynamic settings. In my view that is the point of them. You get this "toggle" behaviour through the options structs, but not when passing arguments when creating the actual underlying object.

💬 TheCharlatan commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#issuecomment-3580678291)
This looks reasonable, Concept ACK

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564471830)
Good observation. I did not think about this asymmetry before. With the recent changes where the transactions are stored in a single container (map) I had to define an explicit "equal to" operator (`struct CTransactionRefComp` in the code). This forces a symmetry between add and remove operations. I chose to consider transactions equal if both txid and wtxid match. This means that now "add" is more relaxed and would allow the addition of same txid, different wtxid. I think that is fine - "why wo
...

💬 fanquake commented on issue "RFC: Add versioning to the kernel header":
(https://github.com/bitcoin/bitcoin/issues/33911#issuecomment-3580729764)
I think anything is probably fine here (at this point), as long as any versioning doesn't become a burden on productivity. I think the API should remain experimental, and no guarantees about backwards compatibility or stability should be made. If the need arises, we should be free to completely rewrite or re-architect (in a breaking way) anything we have released. We currently have a small, reactive group implementing things on top of the API, who I think will be willing to continue to rewrite/a
...

💬 stringintech commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#issuecomment-3580755258)
Concept ACK on being more strict with using `assert`.

I think it makes more sense for a public API to sanitize user input and communicate issues with the input, allowing for recovery and leave usages of `assert` for internal logical inconsistencies (library programmer errors that must never happen).

Regarding the approach: I like what is suggested in https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2561205810 (exposing the enum and having it as the return value). I'm still thinkin
...

✅ stringintech closed a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)

💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3580764433)
Fair points! Thank you for the feedback.

Closing as this is not as helpful as I initially thought.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#issuecomment-3580784708)
Closing - these commits are all merged into #32061 including all the great review feedback <3

✅ pinheadmz closed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747)

👋 pinheadmz's pull request is ready for review: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)

💬 maflcko commented on issue "RFC: Add versioning to the kernel header":
(https://github.com/bitcoin/bitcoin/issues/33911#issuecomment-3580786554)
It is probably fine for the first few releases to just treat the Bitcoin Core `FormatFullVersion()` as the kernel version. It is clear that that version does not follow the semantic versioning of the kernel API, but if kernel users care about breaking changes, they will be listed in the release notes anyway.

💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3580788520)
Ok this PR is back up for review. It is divorced from SockMan, and the I/O loop is now implemented inside `HTTPServer` itself.

💬 stickies-v commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2564545589)
Good catch @l0rinc, I didn't test this in my review, and you're right, it doesn't work (`AttributeError: 'Namespace' object has no attribute 'out_sdkt_path'`) on master nor in the current HEAD. I also agree with removing `nargs=1`:

<details>
<summary>git diff on 266a2bf40e</summary>

```diff
diff --git a/contrib/macdeploy/gen-sdk.py b/contrib/macdeploy/gen-sdk.py
index e9221cf60d..bfac1542a2 100755
--- a/contrib/macdeploy/gen-sdk.py
+++ b/contrib/macdeploy/gen-sdk.py
@@ -21,7 +21,7 @@
...

👍 theStack approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3510402810)
re-ACK 70d9e8f0a15d07a27ae37befb5c1bce71c98d8de

💬 fanquake commented on pull request "depends: latest config.guess & config.sub":
(https://github.com/bitcoin/bitcoin/pull/33945#issuecomment-3580861967)
Guix Build (aarch64):
```bash
50d2ead905b87b4064fd66a0c0a2dc88a7ada65e2224f1ff992bc7ec685e493d guix-build-3e4355314b1a/output/aarch64-linux-gnu/SHA256SUMS.part
923b405f4eb8cb7158f2b4aef0918e48e7e57ef097871d0702dbbd5286013d32 guix-build-3e4355314b1a/output/aarch64-linux-gnu/bitcoin-3e4355314b1a-aarch64-linux-gnu-debug.tar.gz
10317ea4cbaf9b6911592d0d4636d5959bf31f8ad0210cfe8decc4a5ebaa99fb guix-build-3e4355314b1a/output/aarch64-linux-gnu/bitcoin-3e4355314b1a-aarch64-linux-gnu.tar.gz
70bcea
...