💬 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
...

👍 rkrux approved a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636#pullrequestreview-3510456391)
lgtm ACK 217dbbb

Thanks for accepting all the suggestions.

🚀 fanquake merged a pull request: "depends: latest config.guess & config.sub"
(https://github.com/bitcoin/bitcoin/pull/33945)

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#issuecomment-3580889312)
In PR description:

```diff
- primarily adds test coverage for some of the most prominent failure cases in the first commit.
+ primarily adds test coverage for some of the most prominent failure cases in the last commit.
```

⚠️ maflcko opened an issue: "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed."
(https://github.com/bitcoin/bitcoin/issues/33948)
Just ran into this crash in the nightly CI, which was using llvm `22~++20251125085442+ed95c4d6ecf0-1~exp1~20251125085501.1311`. It may be a compiler error, but it is odd that only one test was failing:

https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19692916493/job/56412271437#step:10:6430

```
node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed.
```

...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564638844)
> The first two are just optimizations for logarithmic operations

Just a minor correction, not relevant with the recent changes, but anyway - the first container, "by txid" is where the transactions are stored. The 2nd and 3rd are the indexes. "by priority" is (was) logarithmic and "by node" was `O(1)`.

In the latest push I removed those.

Also, "priority" is something that changes when the transaction is sent more times. So, I went in the direction of storing how many times and when a t
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564640474)
"Resolving" since this code was removed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564641818)
"Resolving" since this code was removed.