💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074860730)
Doesn't the presence of the original cache-usage-update indicate that the comment isn't enough?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074890106)
Same for `NotifyWatchonlyChanged`: It doesn't look to be called anywhere, so can be removed.

👍 maflcko approved a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2817225709)
re-review ACK 1c0d89358e 🎽

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 1c0d89358e 🎽
PZKgfTbviYeE2sXUccefH
...

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074911165)
nit in b6bb744a881d465d15b40413a2753ca4865e851a: Seems fine to fully remove this, given that it isn't needed anyway after commit 1111aecbb58d6e37d430d477ac43f52811fd97d9

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074935902)
nit in a29101b56541d4bf72fcf69460e8eb2a8cc33979: I guess you wanted to leave some of those for follow-ups, but the `doc/descriptors.md` one seems in-scope?

```
$ git grep -i 'legacy wallet' 1c0d89358e12fc871e99c8304d5cb50965cf7b9d doc/descriptors.md doc/managing-wallets.md src/bitcoin-wallet.cpp src/wallet src/script/ test/
1c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/descriptors.md:- `importmulti` takes as input descriptors to import into a legacy wallet
1c0d89358e12fc871e99c8304d5cb509
...

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074950668)
(same commit): remove

```
test/sanitizer_suppressions/tsan:race:BerkeleyBatch
```

?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074924915)
(same commit): Forgot to remove `deprecatedrpc=create_bdb`?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074946960)
Yeah, I meant that flushing isn't used by any chain client and I don't think there is much need to keep the dead code, but can be done in a follow-up.

💬 metasmile commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853693834)
The main arguments supporting the removal of the OP_RETURN size limit appear to be as follows:

- If a transaction includes a large amount of data, it will naturally incur significant fees, which serve as a de facto limitation.
However, it must be seriously considered that the block space can still be abused by certain entities or groups who are indifferent to high fees.

- It has been argued that due to the currently open parameters, OP_RETURN has already been used in a way that is effecti
...

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074994436)
follow-up nit in 18a1bd87813326ac2b1df43b8d37e5815f1ec033: Could convert those to descriptor wallets?

💬 willcl-ark commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#issuecomment-2853762080)
> @willcl-ark I guess you'll have to push the requested change, or reply to it, or close this pull?

Sure @maflcko, I took the suggestion and rebased.

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

```
1c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/rpc/wallet.cpp: {RPCResult::Type::STR, "format", "the database format (bdb or sqlite)"},

1c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/tool_wallet.py: def assert_is_bdb(self, filename):

1c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/lint/lint-locale-dependence.py: "src/wallet/bdb.cpp:.*DbEnv::strerror", # False positive
```

And possibly:

```
sh-5.2$ git grep 'DeleteRecords('
...

✅ maflcko closed an issue: "Intermittent failure in tool_wallet.py in self.assert_tool_output('', '-wallet=salvage', 'salvage') : assert_equal(p.poll(), 0) ; AssertionError: not(3221226505 == 0)"
(https://github.com/bitcoin/bitcoin/issues/30894)

💬 maflcko commented on issue "Intermittent failure in tool_wallet.py in self.assert_tool_output('', '-wallet=salvage', 'salvage') : assert_equal(p.poll(), 0) ; AssertionError: not(3221226505 == 0)":
(https://github.com/bitcoin/bitcoin/issues/30894#issuecomment-2853782378)
For reference, BDB removal https://github.com/bitcoin/bitcoin/pull/31250 was merged (for 30.0)

✅ maflcko closed an issue: "importaddress failed, Only legacy wallets are supported by this command."
(https://github.com/bitcoin/bitcoin/issues/29772)

💬 maflcko commented on issue "importaddress failed, Only legacy wallets are supported by this command.":
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2853789616)
> or closing this as a duplicate of https://github.com/bitcoin/bitcoin/issues/30175

Let's move the discussion to one place

✅ maflcko closed an issue: "DB_RUNRECOVERY: Fatal error, run database recovery"
(https://github.com/bitcoin/bitcoin/issues/29026)

💬 maflcko commented on issue "DB_RUNRECOVERY: Fatal error, run database recovery":
(https://github.com/bitcoin/bitcoin/issues/29026#issuecomment-2853804340)
For reference, BDB removal https://github.com/bitcoin/bitcoin/pull/31250 was merged (for 30.0)

So I presume this was fixed as part of that pull

👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2817471142)
reACK 7238f242d78f3d71834764031d7904d19525bab3

Changes since my last ACK (excl. the rebase) look OK to me.

💬 fanquake commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2075054496)
Please don't add triples with trailing versions numbers, as it's just something else to be "bumped". You can leave the windows example as-is.