💬 maflcko 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-2080852359)
If the transaction is eventually considered abandoned in all cases, then adding the `sync_mempools` (or I guess `syncwithvalidationinterface` would be enough) seems fine.

⚠️ luke-jr opened an issue: ""Migrate Wallet" is unclear to translators"
(https://github.com/bitcoin/bitcoin/issues/29979)
Going through translations, I'm seeing things that suggest "migration" is being misinterpreted in the sense of animals migrating (moving/relocating). We could clarify with translator comments (assuming translators actually see them), but that won't help if non-native English speakers are using Bitcoin Core in English, so maybe a better approach is to come up with another word for this?

💬 m3dwards commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2081071753)
I have tried to look for an alternative variable that will enable verbose output regardless of build system but I couldn't find one. I thought this looked promising [https://cmake.org/cmake/help/latest/envvar/VERBOSE.html](https://cmake.org/cmake/help/latest/envvar/VERBOSE.html) but it didn't work for me.

💬 tdb3 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2081107100)
> > both curl and bitcoin-cli returned an error:
>
> I suspect you're running with `-rpcdoccheck`, you shouldn't get any errors without it? I'm not sure if `-rpcdoccheck` is meant to be an internal-only testing flag (as per #24695), but it [doesn't seem to be documented as such](https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/init.cpp#L655), so I guess better to be safe - force pushed to make the docs contingent on the `-rpcdeprecated` too.
>
> Thanks f
...

⚠️ 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
...