📝 GarmashAlex opened a pull request: "docs: sync external-signer.md with current external signer flow"
(https://github.com/bitcoin/bitcoin/pull/33947)
Updated external-signer.md to match the current implementation. The signer network selection is passed as --chain rather than --testnet, and transaction signing is performed via stdin using the signtx command with JSON output containing a psbt field or error. Wallet creation for external signers requires external_signer=true with private keys disabled, so the example now uses named arguments. Spending with an external signer goes through the new send/sendall RPCs which build a PSBT, call the sig
...

✅ fanquake closed a pull request: "docs: sync external-signer.md with current external signer flow"
(https://github.com/bitcoin/bitcoin/pull/33947)

💬 fanquake commented on pull request "docs: sync external-signer.md with current external signer flow":
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3580498625)
Thanks, however there is an existing PR at #33765. I'd suggest reviewing there first.

🤔 maflcko reviewed a pull request: "fuzz: wallet: add target for `TransactionCanBeBumped`"
(https://github.com/bitcoin/bitcoin/pull/33916#pullrequestreview-3510093058)
needs rebase

💬 maflcko commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564296509)
missing trailing comma?

💬 maflcko commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564297647)
clang format new code?

💬 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