💬 samyan commented on issue "Regtest mode loses unspents after day":
(https://github.com/bitcoin/bitcoin/issues/28262#issuecomment-1678629158)
> Txid `2a98703475934131304b5d1a3cfae4795b8f83709e6965771bdf02f600bfaffc`, created 9 hours ago, spent the `16e0b64735f1e4748a88ef414ec7b70861c4467ffef0dba1e628cc21a312507e : 0` output.

Okay, we're going to check it again. Thanks for your help.

💬 MarcoFalke commented on pull request "blockstorage: Drop legacy -txindex check":
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1678652452)
> though I lack historical context to really judge the second commit

The basic idea is that the legacy txindex was stored inside the block index db. Then a new `src/index/` infrastructure was added and there was migration code. Then, the migration code was removed and a warning was added that a previous version is needed (or a full `-reindex`) to do the migration. Now that all those previous versions are EOL, and thus there should no one be left, who needs a migration, the warning can be remo
...

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1294386567)
> Yea I think this makes sense while we are converting, otherwise I suspect there'll be more churn (similar to the comparisons).

It is fine, if reviewers prefer this. Though, I wouldn't call it "type-safe" and it would be better to at least add a comment about the trade-offs you are making.

> But how would you suggest to forbid this?

It can be forbidden by not allowing it. If you are asking how to do it in C++, something like this should work:

```cpp
class transaction_identifier{

...

💬 MarcoFalke commented on pull request "ci: Move tidy to persistent worker":
(https://github.com/bitcoin/bitcoin/pull/28214#discussion_r1294410716)
Removed in this pull. Should be trivial to re-ACK.

🚀 fanquake merged a pull request: "fuzz: fix a couple incorrect assertions in the `coins_view` target"
(https://github.com/bitcoin/bitcoin/pull/28215)

💬 fanquake commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1678703206)
Want rebase this now that #28215 is merged (also update PR description).

👍 fanquake approved a pull request: "crypto: BIP324 ciphersuite follow-up"
(https://github.com/bitcoin/bitcoin/pull/28267#pullrequestreview-1578298607)
ACK 93cb8f03807dcd3297b7fe18b6c901a8d2a01596 - thanks for following up here.

🚀 fanquake merged a pull request: "crypto: BIP324 ciphersuite follow-up"
(https://github.com/bitcoin/bitcoin/pull/28267)

🚀 fanquake merged a pull request: "ci: Drop no longer needed `macos_sdk_cache`"
(https://github.com/bitcoin/bitcoin/pull/28269)

👍 fanquake approved a pull request: "ci: Use hard-coded root path for CI containers (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28185#pullrequestreview-1578304050)
ACK https://github.com/bitcoin/bitcoin/pull/28185/commits/fafa17c00b87c685200198979ed13018e55e474a - somewhat tested. MSAN changes are the same as what we did for tidy.

🚀 fanquake merged a pull request: "ci: Use hard-coded root path for CI containers (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28185)

💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1678723521)
> A personal thought: A BIP obsoleting change should have a larger and more diverse set of community discussion. Possibly even running it through the mailing list first. As it stands the ACKs/NACKs ratio doesn't even reach 2/3, currently at 64.5% ACK rate

Although there was a debate in particular with the BIP47, we must not forget that some people like Mike are there only to protect their business with the src-20 and stamps. They absolutely do not care about the decentralization of Bitcoin an
...

💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1678730811)
Rebased.

💬 fanquake commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1294438016)
Is this (another) a bug in IWYU? Seems a bit weird to remove the `<event2/event.h>` include, which, as far as I can tell, is correct. It's where `event_base` is defined, and what is [expected when using libevent](https://github.com/libevent/libevent/blob/c9ec6aafb6a025f03636ae2ae7c18a2d4b8a7ed8/include/event2/event.h#L55):
> section usage Standard usage
> Every program that uses Libevent must include the <event2/event.h>
header, and pass the -levent flag to the linker.

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1294444800)
I've reverted back to using `ConsumeDeserializable`. I had the same intuition as @brunoerg but from my testing it's not clear that the custom way is more efficient (in terms of coverage per unit of time). Absent this, it makes sense to not introduce more code and simply use the existing utilities.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1294456468)
Good question. Yes, it should be like this to preserve the behavior from `master` wrt which networks are reachable if `-onlynet` is used. The relevant snippet from this PR:

```diff
if (args.IsArgSet("-onlynet")) {
- std::set<enum Network> nets;
+ g_reachable_nets.RemoveAll();
for (const std::string& snet : args.GetArgs("-onlynet")) {
enum Network net = ParseNetwork(snet);
if (net == NET_UNROUTABLE)
return InitError(
...

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1294456676)
Done, thanks for the suggestion!

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1678810238)
`8b495486e2...be7ae7b1ec`: rebase due to conflicts and address suggestions

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1294501734)
There is no built in construct in the language to do that and I do not like `NET_MAX` which I perceive as a hack.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1294503814)
Since https://github.com/bitcoin/bitcoin/pull/27116 is stalled I have added `AssertLockNotHeld()`, e.g.:

```cpp
AssertLockNotHeld(m_mutex);
LOCK(m_mutex);
m_reachable.insert(net);
```