💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1744573708)
I didn't dig into it but it appeared wallet_tool library disappeared in the conversion to cmake, so I removed references to it here. Probably libraries.md needs to be updated too though. There may be an existing cmake documentation PR where that change could be / has been made

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2330254084)
Updated 76ef8113f70070ab1deeeb142977d46d8132c36e -> 3e4312eef78f233eb7ae1d7d85e497de34144f2e ([`pr/weakcheck.8`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.8) -> [`pr/weakcheck.9`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/weakcheck.8..pr/weakcheck.9)) to fix failing CI job due to changed default build directory (https://cirrus-ci.com/task/5141392714891264?logs=ci#L5325)

💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1744584134)
> I didn't dig into it but it appeared wallet_tool library disappeared in the conversion to cmake...

I couldn’t bring myself to create a library for just a single object file. :)

For the reference, the library was defined as follows: https://github.com/bitcoin/bitcoin/blob/80f00cafdeef0600fa1a5e906686720786c2336c/src/Makefile.am#L547-L549

🤔 furszy reviewed a pull request: "[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks"
(https://github.com/bitcoin/bitcoin/pull/30385#pullrequestreview-2281528955)
> The assumeutxo node would just be a limited node, it'll offer the most recent blocks and none of the history. (it might have more history then it offers, as is the case of any other limited node.)

Yeah, this was the piece I was missing. Thanks!
Funny, I thought about a dynamically adjustable network signal but didn't consider reusing the limited service flag by enabling/disabling full-node service support based on the assumeUTXO background chain-sync status..

Created #30807 implementing
...

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1744622700)
Seems like "echo err 1>&2` should be portable?

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1744623636)
Why is an extra cmd.exe needed?

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1744623828)
Seems like `echo err 1>&2` should be portable?

👍 tdb3 approved a pull request: "doc: fix assumeutxo design doc link"
(https://github.com/bitcoin/bitcoin/pull/30819#pullrequestreview-2281620339)
ACK e5f7272ad322aa4eb7a7d6f531419a4d7aee4802
Good eyes

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744646596)
I'm sorry, I just don't see how those hardcoded tests are any different from the ones I linked. `ZeroL` is the same as `arith_uint256{0}`, and `uint256::ZERO` is the same as `uint256{0}`. Besides these aliases being irrelevant for a conversion test, their equivalence is tested in other unit tests already. What am I missing?

👍 tdb3 approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2281653280)
ACK e9ad9e01409eaa2009f75deb1cf834983c5a6137

Good find, thanks.
Tested similarly to @ismaelsadeeq (ran bitcoind on regtest independently of `p2p_permissions`) and observed the PR branch preventing collision (while the master branch encountered it).

Left a very minor nit. Feel free to ignore.

💬 tdb3 commented on pull request "test: Add explicit onion bind to p2p_permissions":
(https://github.com/bitcoin/bitcoin/pull/30805#discussion_r1744664964)
nit: adding a comment explaining the reason the line was added could help prevent regression in the future.

e.g.
```
# explicitly bind the tor port to prevent collision with the default port
```

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2330411726)
@gmaxwell

> Huh? Nothing here changes at all what transactions a node relays. And someone can always waste bandwidth to you if thats their only goal (e.g. sending you IP ping packets ).

> In any case, I'm only concerned about the shortest path to deploy-- since the behavior change could be made initially lax and then tightened over time. Right now, existing bitcoin core nodes are foolishly making many duplicate reque> sts. It's clear that this could be improved, but it seems like some will
...

🤔 tdb3 reviewed a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817#pullrequestreview-2281665391)
Approach ACK

Nice addition. Definitely don't network broken from a failed RPC.
Keeping the PR scoped to just `NetworkDisable` seems good (more concise to reviewers).

💬 tdb3 commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#discussion_r1744674413)
Could use `node.blocks_path` instead of `node.chain_path` for these two lines.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2330427591)
> I think the mass connector concern would be potentially addressable by doing something like, "if a peer sends an unsolicited txn stop all processing on the peer for N seconds" (h/t to Pieter for mentioning that in an off-thread disc> ussion).

I don't think it is that easy...a peer can be the fastest to announce a seen-by-all network txid, get requests from the node
and then inject an unsolicited tx to stop all processing, to force the node to fallback to another peer to fetch the seen-by-
...

💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1744714899)
pushed.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2330483767)
Updated at [23551ef] with the `NODE_TXRELAY_V2` approach.

Submitted a BIP [draft](https://github.com/bitcoin/bitcoin/pull/30572/commits/23551efc912f912c2ccedde9b6518854dad7aac9) in that sense, in the lack for now of a better approach for now on how to implement reject of unrequested transactions in a minimally-disruptive fashion for the bitcoin peer-to-peer network. I think I should open a distinct PR for the service signaling support itself.

💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2330506782)
> unsure how to get to the main chain

You are on the main chain. But the deep reorg didn't kick in yet, so if you use for example mempool.space to see, if you are on the main chain or not, then that page is still showing the old chain. I guess it can take a few days to get there, but we are getting closer and closer, to trigger that reorg.

💬 maflcko commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#discussion_r1744793926)
style nit: Pathlib should have the corresponding call as a member method. Using that would avoid the `os` import.

💬 maflcko commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1744801683)
Please don't re-introduce https://github.com/bitcoin/bitcoin/issues/30608