💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2367755093)
Thanks! Fixed.

💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3318353570)
The [feedback](https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2367130713) from @hodlinator has been addressed.

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

Some users clearly still want this option and keeping this option in doesn't negatively impact any other users nor make the code harder to maintain than having the option marked as deprecated. It's a really bad idea to alienate users over this.

I also suggest making it clear to endusers as well as packagers and node-in-a-box vendors that this option is still around and to make it easy for them to use it. Regardless of whether we think using this option has the purported effects
...

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

I agree that removing the deprecation does not have a major effect on users, and it also gives users who want to use this flag a signal that it will not be removed in a future release. I see no issue in offering flexibility for node runners.

Is this able to be added to the v30 milestone? https://github.com/bitcoin/bitcoin/milestone/72

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

* bitcoin core is not here to be everyone's everything implementation. It's here to be the best software for operating the network.
* Adding, or in this case not removing, code with only negative effects on the actual operation of users' nodes is not responsible stewardship of bitcoin.
* Bowing to demands from people who do not understand what they are asking for (as repeatedly demonstrated in online argumentation) is not how any successful project can operate.

These issues
...

💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3318413899)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/31802.

💬 0xB10C commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3318432100)
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. They mention: `Both -datacarrier and -datacarriersize options have been marked as deprecated and are expected to be removed in a future release.`

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