💬 RandyMcMillan commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3318573615)
Concept ACK

🤔 Eunovo reviewed a pull request: "log: always print initial script verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3252294836)
Left a few comments

💬 Eunovo commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2367994445)
https://github.com/bitcoin/bitcoin/pull/33336/commits/c99fde2c0ac181da656e8c942fa6fea93713de75: wouldn't it be better to just do `std::optional<std::string> script_check_reason`?

💬 Eunovo commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2367997436)
https://github.com/bitcoin/bitcoin/pull/33336/commits/c99fde2c0ac181da656e8c942fa6fea93713de75: I think this was better as `std::optional<bool>`

💬 vasild commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2368062584)
Will do if I retouch. Change applied locally.

💬 RandyMcMillan commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3318711118)
you should probably revert

https://github.com/bitcoin/bitcoin/pull/32406/commits/63091b79e70b8e230a122fa6fb3dac91c80638e7

as well

💬 ajtowns commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3318779164)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3

I don't think getting upset about the presence or absence of a deprecation note is sensible, so my ACK shouldn't be taken as an indication that I'll be upset if this PR isn't merged.

There were a few rationales given for deprecation, I think the strongest being:

* the philosophy that "all user options should either come with clear recommendations for how to use them or, if no recommendation can be made, they should not exist or be discourage
...

💬 kristapsk commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3318829265)
Concept ACK

👍 TheCharlatan approved a pull request: "multiprocess: Don't require bitcoin -m argument when IPC options are used"
(https://github.com/bitcoin/bitcoin/pull/33229#pullrequestreview-3252708178)
Re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de

🤔 janb84 reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3252732839)
Concept ACK 07027afa155f8ec9715c761d80630b31723c2c32

NIT: The separate commit has no additional value, please squash them.

Checked that
- the bash script halts if python script exits with code 1 ✅
- All the binaries are checked by the python script ✅

<details>
edited the python script to check that all the binaries are being checked (that the build script calls the security python check correctly)
```shell

Checking binary security on installed executables...

Checking /dists
...

🤔 danielabrozzoni reviewed a pull request: "net/rpc: Report inv information for debugging"
(https://github.com/bitcoin/bitcoin/pull/33448#pullrequestreview-3252776467)
ACK 77b2ebb811824899f56976f8e3113914706edc97

Code looks good to me, I have done some minimal testing on my own node (running `getpeerinfo` and inspecting the result)

Noteworthy change: the PR changes the mutex that guards `m_last_inv_sequence`: previously it was `g_msgproc_mutex`, but since the mutex is for "anything that is only accessed via the msg processing thread", this PR changes it to `m_tx_invetory_mutex`. I manually checked that in any place where we use `m_last_inv_sequence`, the
...

💬 maflcko commented on pull request "net/rpc: Report inv information for debugging":
(https://github.com/bitcoin/bitcoin/pull/33448#issuecomment-3319094519)
lgtm, but could add a small test? Maybe by modifying `test_tx_in_block`, or by copy-pasting `test_tx_in_block` into a newly created sub-test? A hacky diff:

```diff
diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py
index a1a00751d1..54eaaef7ab 100755
--- a/test/functional/p2p_leak_tx.py
+++ b/test/functional/p2p_leak_tx.py
@@ -36,8 +36,17 @@ class P2PLeakTxTest(BitcoinTestFramework):

self.log.debug("Generate transaction and block")
inbound
...

💬 instagibbs commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319141087)
@bitschmidty to be clear are you considering this as something to backport (30 et al), or just for following releases from master.

Assuming this is merged no one who will bother setting this argument in 30.0 will be unable to find relevant drama and the resolution of the PR in the affirmative.

concept NACK on backport, truly ambivalent on undeprecating because I can see how it's confusing to honestly curious users to not have a timeline to do so (and maybe bad project hygiene), but also it
...

💬 darosior commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319149396)
I think Bitcoin Core should strive to limit its extensive and growing number of startup options. Optionality for the sake of it is counterproductive. On this basis, the `-datacarrier` and `-datacarriersize` options controlling a long obsolete relay policy limit are in my opinion prime candidates for removal.

That said, certain users care strongly about using those options. In these conditions, i do not see the project removing the option anytime soon. Therefore i think it's technically incorr
...

📝 vasild opened a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454)
Normally `ConnectNode()` could choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.

Document both functions.

This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.

---

This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting i
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3319219472)
I extracted one commit from here into https://github.com/bitcoin/bitcoin/pull/33454 with the aim to reduce the size of this PR once #33454 is merged.

💬 0xB10C commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319240689)
> > fwiw: If/when this is merged, the [draft release notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#policy) should be updated.
>
> Well, only if it gets backported to 30.0?

Correct, I've edited my comment to reflect that.

💬 benthecarman commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3319342245)
@stwenhao i don't think restricting the nonce adds any meaningful protection. The attacker can also use the merkle root as a nonce field because they do not need to actually include txs. If you limit the nonces, they can just change the merkle root until they find a block

💬 bitschmidty commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319393851)
> are you considering this as something to backport (30 et al)

That is preferred IMHO. But that is the judgment of you all and those responsible for the release process (assuming this PR gets support).

📝 purpleKarrot opened a pull request: "CPack"
(https://github.com/bitcoin/bitcoin/pull/33455)
The goal is to build all packages (both archives and installers) from a single configuration in CMake.

### Packaging Instruction

Get a list of supported CPack generators for your platform with:

```sh
cpack --help
```

Run `cpack` from the build directory after a successful build to select multiple package generators:

```sh
cmake --build .
cpack -G '7Z;NSIS64'
```

Or build the `package` and/or `package_source` targets:

```sh
cmake --build . --target package
cmake --buil
...