💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2060783455)
Removed

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2831273073)
> There's also a `libdb++-dev` instance in `workflows/ci.yml` & `deadlock:libdb` / `BerkeleyBatch` etc in tsan suppressions. `Berkeley*` in walletdb.h. `BerkeleyEnvironment::Salvage` in `utils_tests.cpp`.

Removed

💬 amtriorix commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2831298379)
I do disagree with this merge
Are you crazy or what. Backwards compatibility is needed for wallet.dat

💬 jonatack commented on issue "Moving this repo to bitcoin-core":
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2831313159)
ACK after reading the above, the IRC meeting discussion, and @achow101's gist.

🤔 darosior reviewed a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-2795154468)
Concept ACK. Will test using my Rust client for #29409 rebased on top of this.

💬 theuni commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#issuecomment-2831335989)
Looking over the old code, I believe it used to be true that `-connect`, `-addnode`, etc. used `m_addr_name` (and a dummy for `addr`) because they may have required a dns lookup. Which would have meant that checking both was required. There was also the case where we were connecting to an address (which may or may not need to be resolved) through a proxy.

These days I believe we're smarter and detect addresses which don't need to be resolved.

I'm pretty sure that `addr` should match `m_add
...

🤔 jonatack reviewed a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338#pullrequestreview-2795181683)
Concept ACK

💬 jonatack commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#discussion_r2060824325)
> we'd want to know if that didn't hold for some reason

Wonder if the code should assert they are the same, for debug builds.

💬 l0rinc commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2831353392)
> miniscript_tests (SEGFAULT)

This was the original failure in debug mode

> [Here](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14667837569/job/41166981753) is a "Windows, Debug" CI job

While the build is till running, this test passed alerady:
```python
89/140 Test # 60: miniscript_tests ..................... Passed 126.23 sec
```

> Should this marked as Fixes https://github.com/bitcoin/bitcoin/issues/32341?

Added it to the description

💬 theuni commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2831361039)
Is making `m_nodes_mutex` non-recursive a goal? That sounds nice to me and I'm not opposed, but I'd rather not make changes just to get halfway there.

Concept ACK on turning the `FindNode`s into bools, though. That's a nice improvement.

💬 fanquake commented on pull request "build: Fix `macdeployqtplus` after switching to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/32287#discussion_r2060882568)
Same Q here as above.

💬 glozow commented on issue "estimateSmartFee error: "Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/32178#issuecomment-2831440563)
I think there have been a few debates about whether it's safer to return nothing ("insufficient data") vs a potentially bad guess (mempool min could also be artificially low if we're just starting out). But also, lowballing isn't the end of the world since you should be able to replace transactions that didn't pay enough.

💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2831451724)
Needed to reorg the commits a bit to get the test-every-commit CI to pass

💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2831459829)
I guess this could use a bug label too if someone with permission can get around to that :)

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2060900943)
(Done in latest push from ~8 hours ago).

📝 kevkevinpal opened a pull request: "tests: Added progress tracker when running fuzz test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/32354)
Right now if you run `test_runner.py` on all the targets it is hard to tell how far you are in the progress even with `--loglevel=DEBUG` on. As it just spits out the target that was completed an no info on how many targets are left.

This change gives you a percentage in 5% increments on how far along you are finishing the targets you've selected

This also applies when you use `-g/--generate` and `--m_dir`

I can append this change to either change the percent increments or move it to th
...

💬 jonatack commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2831574347)
ACK 4b6171982a20d736b2c627ab9b7ea788b06af457

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061100509)
In commit "refactor: Operate on bytes instead of bits in Asmap code"

This is not correct. The code (and comment) above indicate that `buffer` is being interpreted using 1 bit of asmap code per byte. The line here is interpreting it as 1 byte per byte.

You need some conversion function, I think:

```c++
namespace {

std::vector<std::byte> BitsToBytes(std::span<const uint8_t> input) noexcept
{
std::vector<std::byte> ret;
uint8_t next_byte{0};
int next_byte_bits{0};

...

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061112524)
In commit "refactor: Use span instead of vector for data in util/asmap"

No need for the explicit conversion:
```diff
-if (SanityCheckASMap(std::span<const std::byte>(asmap), buffer.size() - 1 - sep_pos)) {
+if (SanityCheckASMap(asmap, buffer.size() - 1 - sep_pos)) {
```

Also twice the same further in this function.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2061096624)
In commit "refactor: Operate on bytes instead of bits in Asmap code"

I don't think this is necessary? The code above could operate on `asmap` instead of `asmap_vec` directly?