💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1581915244)
Thanks @kevkevinpal. Good example of why review is important.

Your findings nudged me to add some assert test statements to help cover cases involving IPv6 hosts.

We're experiencing the result of the scope of `SplitHostPort()`. `SplitHostPort()` extracts the host and port. Although the port is validated, host is not (and the function declaration comment header mentions validating the port, but doesn't mention validating the host).

https://github.com/bitcoin/bitcoin/blob/7fee0ca014b15
...

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581915794)
Yeah, the explicit check makes it safe. I would just always prefer the safer method and I find it more expressive. But no big deal.

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2081217008)
re-ACK fa737100af538f7257e55d88fe1f24246bcce7fc

> I think assert_equal can generally not be used on floating point values, no?

Here is a version with `assert_approx`: https://github.com/fjahr/bitcoin/commit/24e9a37fb502092048206967dfc792d6af126316

📝 srvinii opened a pull request: "Add CheckScriptPushSize to validate script push data size"
(https://github.com/bitcoin/bitcoin/pull/29981)
This pull request adds a new function `CheckScriptPushSize` to the transaction validation logic to ensure that the push data within script elements does not exceed the maximum allowed size, as defined by `MAX_SCRIPT_ELEMENT_SIZE`.

### Motivation and Context
Scripts within transaction outputs are not currently checked for the size of their push data elements. This oversight could potentially allow for scripts with oversized data elements, which might introduce security risks and non-standard
...

💬 sipa commented on pull request "Add CheckScriptPushSize to validate script push data size":
(https://github.com/bitcoin/bitcoin/pull/29981#issuecomment-2081264726)
This is a consensus change, which means it need discussion and agreement from the community before it can be considered in this repository.

That aside, I don't understand the motivation here. UTXOs with oversized pushes in their scriptPubKeys are unspendable anyway.

💬 srvinii commented on pull request "Add CheckScriptPushSize to validate script push data size":
(https://github.com/bitcoin/bitcoin/pull/29981#issuecomment-2081265708)
> This is a consensus change, which means it need discussion and agreement from the community before it can be considered in this repository.
>
> That aside, I don't understand the motivation here. UTXOs with oversized pushes in their scriptPubKeys are unspendable anyway.

I understand your concern regarding the consensus nature of the change and the implications it could have without broad community agreement. The motivation behind introducing a check for oversized data pushes in scriptPub
...

💬 sipa commented on pull request "Add CheckScriptPushSize to validate script push data size":
(https://github.com/bitcoin/bitcoin/pull/29981#issuecomment-2081266170)
We could simply remove them from the UTXO set if such outputs started to make a meaningful impact. Since they're already unspendable, that would not be a consensus change.

💬 srvinii commented on pull request "Add CheckScriptPushSize to validate script push data size":
(https://github.com/bitcoin/bitcoin/pull/29981#issuecomment-2081267055)
> We could simply remove them from the UTXO set if such outputs started to make a meaningful impact. Since they're already unspendable, that would not be a consensus change.

Considering this alternative, I agree that pursuing a consensus change may not be necessary if the impact of these unspendable outputs can be managed through more straightforward means. I appreciate your insight into this matter and the practical solution you've proposed.

Would you recommend any specific strategies or
...

💬 ajtowns commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-2081267056)
Rebased over #28834, removed redundancy from `LogWarning("Warning: ..")`

💬 theStack commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#discussion_r1581980553)
See the last commit where the numbering is removed. Updating them is also possible of course, but it seems that these numbers don't provide any value.

✅ fanquake closed a pull request: "sync: improve CCoinsViewCache ReallocateCache"
(https://github.com/bitcoin/bitcoin/pull/28945)

✅ srvinii closed a pull request: "Add CheckScriptPushSize to validate script push data size"
(https://github.com/bitcoin/bitcoin/pull/29981)

✅ fanquake closed an issue: "build: Enable Fuzz binary in MSVC"
(https://github.com/bitcoin/bitcoin/issues/29760)

🚀 fanquake merged a pull request: "build: Enable fuzz binary in MSVC"
(https://github.com/bitcoin/bitcoin/pull/29774)

💬 Randy808 commented on issue "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError":
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2081321444)
Sounds good, I'll add `sync_mempools()` to mitigate the immediate inconsistency observed with the `abandontransaction` call. I'll mention the potential race conditions between the signals and wallet operations, but I'm thinking that could be treated as a separate issue.

📝 Randy808 opened a pull request: "test: Fix intermittent issue in wallet_backwards_compatibility.py"
(https://github.com/bitcoin/bitcoin/pull/29982)
When creating and replacing a transaction using `bumpfee`, an async update is sent in the form of the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` signals. When `wallet_backwards_compatibility.py` creates `tx3_id` this way and replaces it with `tx4_id`, the `abandontransaction` rpc is called right after. In some cases the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` is handled after the transaction is abandoned in the wallet, and overwrites the transaction's
...

💬 hebasto commented on pull request "Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/28526#issuecomment-2081336549)
> Based on #24773.

Deferring to after cmake.

✅ hebasto closed a pull request: "Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds"
(https://github.com/bitcoin/bitcoin/pull/28526)

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2081338161)
Rebased to resolve conflicts with the merged bitcoin/bitcoin#29774.

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2081338871)
cc @achow101 @theStack

> Code changes look reasonable, but Windows stuff is not my expertise...

cc @sipsorcery :)