💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2092931817)
Passing `-1717429609` would make this code try to access `weekdays[-1]` :fire:

For modern style, better use `std::array` instead of C arrays and, more importantly, use the array's `at()` method which has boundary checks, just in case. Here is a change that adds some more tests and fixes the out-of-bounds access:

<details>
<summary>[patch] FormatRFC7231DateTime()</summary>

```diff
diff --git i/src/test/util_tests.cpp w/src/test/util_tests.cpp
index 387493152d..ebb40dd713 100644
---
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093076600)
e95c6f5b6511ae35141b1e440e1f22e1004d3de6

Copying strings is expensive:

```suggestion
std::optional<std::string_view> HTTPHeaders::Find(const std::string& key) const
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093078641)
```suggestion
void HTTPHeaders::Write(const std::string& key, const std::string& value)
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093181637)
Would be good to document when `Read()` throws, returns true and false.

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2846448811)
Posting review midway. Reviewed up to and including e95c6f5b6511ae35141b1e440e1f22e1004d3de6 `http: Implement HTTPHeaders class`

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093081846)
```suggestion
void HTTPHeaders::Remove(const std::string& key)
```

💬 jonatack commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886931577)
Concept ACK

💬 mzumsande commented on issue "Change functional tests to trigger reorg by reconsiderblock":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2886940689)
> We should make sure that in our reorg/mempool tests spread across the codebase that we don't do it this way and instead:
>
> 1. Prep fork chain of sufficient length
> 2. invalidate the fork chain
> 3. do normal test setup
> 4. reconsiderblock the previously invalidated chain
>
> This appears to match the intended test coverage.

Why not simply submit the fork chain at the right point in time, provided it has more work - no invalidateblock / reconsiderblock involved at all. I always thought t
...

📝 theStack opened a pull request: "test: properly check for per-tx sigops limit"
(https://github.com/bitcoin/bitcoin/pull/32533)
Currently the per-tx sigops limit standardness check (bounded by `MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops" if exceeded):
https://github.com/bitcoin/bitcoin/blob/3f83c744ac28b700090e15b5dda2260724a56f49/src/validation.cpp#L925-L927

is only indirectly tested with the much higher per-block consensus limit (`MAX_BLOCK_SIGOPS_COST`):
https://github.com/bitcoin/bitcoin/blob/3f83c744ac28b700090e15b5dda2260724a56f49/test/functional/data/invalid_txs.py#L236-L242

I.e. an i
...

💬 instagibbs commented on issue "Change functional tests to trigger reorg by reconsiderblock":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2886947178)
> Why not simply submit the fork chain at the right point in time

Sure, that works too, it just seemed easiest since you can use ~all the tooling to generate blockchains. e.g., if you want stuff in the fork blocks it's trivial to do via rpc.

💬 sipa commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886989763)
> After this PR, it will mean your chain tip's timestamp is within 2 hours of now.

I think that's a worse cure than the disease. In case we **know** there are unprocessed blocks in the chain (because we have their headers), we shouldn't say progress: 1.

🤔 pablomartin4btc reviewed a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2845981748)
It seems that `wallet_backwards_compatibility.py` is being skipped in the CIs?


And I got this error when I run it locally:

```
2025-05-16T13:44:41.540000Z TestFramework (INFO): Testing inactive hd chain bad derivation path cleanup
2025-05-16T13:44:43.146000Z TestFramework (INFO): Test that legacy wallets are ignored during startup on v29+
2025-05-16T13:44:43.592000Z TestFramework (INFO): Stoppin
...

💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2092636812)
nit: I'd use the same ref to the `node_master` as above...
```suggestion
self.restart_node(node_master.index, extra_args=[])
# Verify node is active after restart
node_master.getblockcount()
```

💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093187305)
I think something like this (or within a function called `assert_restart_raises_init_warning`):
```suggestion
# Restart latest node and verify that the legacy wallet load is skipped with a warning during init.
with node_master.assert_debug_log([f"Failed to open database path '{wallet_path}'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option)."]):
self.restart_node(node_master.index, extra_args=[])

...

💬 fanquake commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#issuecomment-2887005738)
> It seems that wallet_backwards_compatibility.py is being skipped in the CIs?

It is run in the previous releases CI, which is the one that is failing in this PR: https://github.com/bitcoin/bitcoin/runs/42313448794.

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2887006170)
Thanks for the reviews and for splitting out `prevector`!
A follow-up that reenables the coins cache sanitizers by fixing the used cache accounting is done in https://github.com/bitcoin/bitcoin/pull/32313

💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#issuecomment-2887006977)
> > Is it worth it to verify that, previous to this PR, the node with the legacy wallet in its `settings.json` was failing to restart (using `assert_start_raises_init_error`)?
>
> That's what is being tested. You can run the test on master and it will fail due to that behavior.

I meant without switching to `master` and once this PR gets merged, I thought there was a release with this failure and your fix will be in the next one, but both changes will be part of the same release so the answ
...

💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093241973)
I've left a [suggestion](https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093187305).

💬 mzumsande commented on issue "avoid using invalidateblock to directly test reorg behavior":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2887013040)
> > Why not simply submit the fork chain at the right point in time
>
> Sure, that works too, it just seemed easiest since you can use ~all the tooling to generate blockchains. e.g., if you want stuff in the fork blocks (and make as many blocks as you like) it's trivial to do via rpcs like generateblock.

Oh ok I see now, as in https://github.com/bitcoin/bitcoin/pull/32516/commits/16af944a547bfea8f1f910938491a8ad11a47de5. Could also prepare the fork chain on a second node using whatever toolin
...

📝 fanquake opened a pull request: "Update leveldb subtree to latest upstream"
(https://github.com/bitcoin/bitcoin/pull/32534)
Pulls in
* https://github.com/bitcoin-core/leveldb-subtree/pull/51

Remove the related warning suppression.