⚠️ hebasto opened an issue: "depends: Cross-compiling `qt` for `arm-linux-gnueabihf` fails"
(https://github.com/bitcoin/bitcoin/issues/29980)
On Ubuntu 24.04 with glibc 2.39:
```
compiling ../3rdparty/zlib/src/gzclose.c
In file included from /usr/arm-linux-gnueabihf/include/features.h:394,
from /usr/arm-linux-gnueabihf/include/bits/libc-header-start.h:33,
from /usr/arm-linux-gnueabihf/include/stdio.h:28,
from ../3rdparty/zlib/src/gzguts.h:40,
from ../3rdparty/zlib/src/gzclose.c:6:
/usr/arm-linux-gnueabihf/include/features-time64.h:26:5: error: #error "_TIME_BI
...

💬 fjahr commented on pull request "refactor: extract CCheckQueue's data handling into a separate container "Bag"":
(https://github.com/bitcoin/bitcoin/pull/27331#issuecomment-2081161297)
I didn't dive too deeply into this yet but it's a bit sad that we can not remove more code due to this, that would certainly make it a more interesting change. Do you already have other places in mind where we could use this?

You also mention the order of elements, but it's unclear why that is important. This hasn't changed right? Or is this just about the naming choice?

I can't really make sense of the CI failure. Could you do a rebase? Since this is a fairly old change it might be some h
...

🤔 fjahr reviewed a pull request: "sync: improve CCoinsViewCache ReallocateCache"
(https://github.com/bitcoin/bitcoin/pull/28945#pullrequestreview-2026733390)
@martinus Are you still working on this? If yes, it would be great if you could rebase and address @andrewtoth 's comment. If not, I can try to take this over and get it the attention it deserves.

💬 fjahr commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#discussion_r1581896321)
Should probably add something about this to the docstring.

💬 martinus commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953)
@fjahr I'm not working on this any more, feel free to take over!

💬 fjahr commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#discussion_r1581902620)
The numbering of this list is off after this change. That might be confusing to someone in the future. I would suggest either updating the following numbering or something like.

```
* 6. (deleted)
* 7. (deleted)
```

💬 brunoerg commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2081177346)
Concept ACK

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2081190733)
> Concept ACK. I think the pull description can be rewritten, since it ends up in plain text in the merge commit. Instead of writing "Thing A is not included. It is now", it would be better to say why it is included and remove the formatting/negation: "Thing A is included, because (...motivation...)"

I have updated the description.

💬 davidgumberg commented on pull request "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime":
(https://github.com/bitcoin/bitcoin/pull/29872#discussion_r1581912776)
nit:

```suggestion
void SetMockTime(int64_t mock_time_in) { SetMockTime(std::chrono::seconds{mock_time_in}); }
```

💬 davidgumberg commented on pull request "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime":
(https://github.com/bitcoin/bitcoin/pull/29872#issuecomment-2081205293)
crACK https://github.com/bitcoin/bitcoin/pull/29872/commits/fae0db555c12dca75fb09e5fa7bbabdf39b8c1df

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