💬 MarcoFalke commented on issue "Validation of malformed address fails with a peculiar message":
(https://github.com/bitcoin/bitcoin/issues/27723#issuecomment-1558620137)
Thank you for the bug report. I think what happens is:

* The function `ConvertBits<5, 8, false>` fails, however
* the function `bech32::LocateErrors` also fails to locate any errors.

Thus the `CHECK_NONFATAL(isValid == error_msg.empty())` added in commit 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e will be hit.

💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1201618467)
You can just use `DataStream` for that, which comes with compile-time checks to enforce that both values are in fact never read, and thus can be omitted completely.

Also, in the same commit, is there a reason why you are changing `DataStream ssKey` to `CDataStream ssKey`? That seems like a step backward, no?

💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1558676764)
> Has that idea been discussed somewhere already?

The idea was published quite a while ago, but I learned about it just recently.
We looked around to see if there were existing discussions or implementations, but we didn't find anything on bitcoin-dev or the repo here.
There's an old implementation in a Bitcoin fork (Drivechain) which I used as a starting point, but ended up with a pretty different approach and implementation.
We were thinking of perhaps writing down the spec into a Pro
...

📝 willcl-ark opened a pull request: "build: disable boost multi index safe mode in debug mode"
(https://github.com/bitcoin/bitcoin/pull/27724)
Fixes #27586

Disable boost multi index safe mode by default when configuring with
--enable-debug.

This option can cause transactions to take a long time to be accepted
into the mempool under certain conditions; iterator destruction takes
O(n) time vs O(1) as they are stored in a singly linked list. See
27586 and the [boost docs](https://www.boost.org/doc/libs/1_58_0/boost/multi_index/detail/safe_mode.hpp) for more information.

Adds a new configure option `--enable-boostmultiindexsaf
...

💬 hebasto commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558736106)
Somewhat related: #27353.

💬 willcl-ark commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558738915)
The approach taken here was influenced by discussion in https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1556705320

I've been unable to instigate the congested behaviour on two nodes overnight, one with default mempool and one with 500MB mempool, but hopefully will manage to do so shortly and will reply here if so.

In any case, it seems overkill to enable these safe mode checks in debug builds (which are often tested on mainnet), and a more sensible approach to limit to CI task
...

💬 fanquake commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558740250)
Concept NACK - I don't think adding configure flags for specific Boost defined is a good approach. Will follow up in the other threads.

💬 willcl-ark commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558764043)
> Concept NACK - I don't think adding configure flags for specific Boost defines is a good approach. Will follow up in the other threads.

OK. Looking forward to your thoughts, as I don't know how the same effect could be achieved in any neater or more maintainable way.

One alternative could be to manually append to the CI CPP flags, if that's all we wanted.

💬 MarcoFalke commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558819623)
> One alternative could be to manually append to the CI CPP flags, if that's all we wanted.

Or just a configure setting to append CPP flags to the existing ones, instead of replacing the default ones?

Concept ACK

💬 fanquake commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1558827148)
> Or just a configure setting to append CPP flags to the existing ones,

There's no need for a new setting. Just use `CPPFLAGS="-DWHATEVER"`. This has worked since #24391.

💬 MarcoFalke commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#discussion_r1201881964)
I wonder if this should be moved to the debug fuzz task to avoid only fuzzing a debug version of boost in all fuzz configs?

💬 willcl-ark commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#discussion_r1201884441)
I like that. If anything it makes more sense to me to fuzz the non debug version...

💬 MarcoFalke commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#discussion_r1201886659)
ideally both are fuzzed :)

👍 MarcoFalke approved a pull request: "build: disable boost multi index safe mode in debug mode"
(https://github.com/bitcoin/bitcoin/pull/27724#pullrequestreview-1439114903)
lgtm, but the description needs to be adjusted?

⚠️ 137718 opened an issue: "Do you want to rebuild the block database now?"
(https://github.com/bitcoin/bitcoin/issues/27725)
Do you want to rebuild the block database now?

✅ fanquake closed an issue: "Do you want to rebuild the block database now?"
(https://github.com/bitcoin/bitcoin/issues/27725)

👍 ajtowns approved a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1438918136)
ACK 3b57b2b8d958871bec9a33953be051a75d8bf07c

Despite thinking this is an improvement on what we currently do and that it's ready for merge, there's two things I'm not 100% comfortable with, and that I think would be good to improve in a followup. (1) I think this puts more demand on outbound peers than it should, as per [previous comments](https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200426349). (2) I think the multimap stuff makes this hard to understand, and it would be good t
...

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1201730942)
You just iterated over all the peers requesting this block, and returned early if one of them was this node. What's the benefit of looping again?

📝 jonathanbier opened a pull request: "Update policy.cpp"
(https://github.com/bitcoin/bitcoin/pull/27726)
I think this should be virtual bytes? Or maybe not

💬 glozow commented on pull request "Update policy.cpp":
(https://github.com/bitcoin/bitcoin/pull/27726#issuecomment-1558979194)
Looks correct to me, cc @Xekyo