💬 sipa commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566641643)
Is there a need to abort the fuzz test when this happens (here and below), or could this be a `break` to allow the test to proceed still, without the transaction that would make the total overflow?

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058142382)
Rebased.

🤔 tdb3 reviewed a pull request: "netbase: clean up Proxy logging"
(https://github.com/bitcoin/bitcoin/pull/29882#pullrequestreview-2002495493)
ACK for d1af4422d13ba84253a1555c2fe5a42170fa8b35
Good cleanup, making logs less cluttered while keeping relevant connection state logging.

In addition to observing the lack of `Cannot connect to socket for` message, also ran a quick test where Tor was stopped then started again. bitcoind was still able to detect and inform the user of the loss of the proxy and subsequent `peer connected` message provided indication that connection through proxy was reestablished.

```
13308 tx=3456746 da
...

💬 tdb3 commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566647446)
I could be mistaken, but it looks at first glance that this would be covered in `ConnectToSocket()`:

https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L603

and reached in Proxy::Connect() through either the call to `ConnectToSocket()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L653

or by way of `ConnectDirectly()`:
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a4
...

💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058216588)
> Is it possible that the wallet files were last opened in an unclean shutdown, or maybe with older versions that didn't always flush on shutdown?

Yes, this is possible, it's a random grab bag of wallet database files (not even sure there's not a litecoin or dogecoin one in there).

> Also, yes, having duplicate files will result in the same unique id which means that the log files can be applied to the wrong file, which will really mess things up.

Honestly after this train wreck i'm at
...

💬 laanwj commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566708786)
Yes, it's not strictly currently necessary, but code changes sometimes. It's good to have error and unexpected situation checking in place, instead of crashing later on.

📝 OkSang88 opened a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that impr
...

✅ OkSang88 closed a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)

📝 OkSang88 reopened a pull request: "BTC world"
(https://github.com/bitcoin/bitcoin/pull/29885)
`<!--`
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that impr
...

✅ laanwj closed a pull request: "BTC team"
(https://github.com/bitcoin/bitcoin/pull/29885)

👋 fanquake's pull request is ready for review: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876)

📝 fanquake locked a pull request: "(deleted)"
(https://github.com/bitcoin/bitcoin/pull/29885)

💬 laanwj commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058276413)
Thanks for your first contribution!

As it is hard to review PRs that change things all over the place, this is unlikely to be merged this way. I'd suggest keeping the code change focused. For example, make it add test cases only, and remove the changes (refactoring and otherwise) to `strecodings.cpp` and other non-test code.

💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2058298297)
You wee right: with the LSN check, one of the wallets from 2015 trips up:
```
terminate called after throwing an instance of 'std::runtime_error'
what(): LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2058305830)
> There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that.

That's good to have. It does look like the newly added check trips up in the CI in some places:
```
unknown location(0): fatal error: in "db_tests/db_cursor_prefix_range_test": class std::runtime_error: LSNs are not reset, this database is not completely flushed. Please reopen then close the database with a version that has BDB support
```

💬 laanwj commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2058329366)
This is something that we should have done a long time ago imo. I wouldn't be surprised if I have an ancient PR doing just this.
Concept ACK!

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058381680)
Hey @laanwj, thanks for your first review.
Please see the commits separately, don't worry, they're not all over the place.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566832562)
Why this change? This style change makes the code harder to read.

Unless there is a reason to change the code and review it, it shouldn't be changed.

💬 maflcko commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058390006)
@paplorinc If you are not being receptive to reviewers' feedback, it is unlikely that anyone will review the pull request and without review it will not be merged.

💬 paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1566838275)
I've changed it to unify it with the previous lambda. Why would I leave the type there when I've used auto 2 lines above?