💬 RandyMcMillan commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503527508)
Concept ACK

Having more control over custom signet parameters/conditions seems really useful IMO.

💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#issuecomment-1503551017)
> I'm wondering if as a followup, we could enable a DEBUG build of libc++
in our MSAN CI job?

sgtm

🚀 fanquake merged a pull request: "fuzz: Add HeadersSyncState target"
(https://github.com/bitcoin/bitcoin/pull/26662)

📝 fanquake opened a pull request: "ci: build libc++ in DEBUG mode in MSAN jobs "
(https://github.com/bitcoin/bitcoin/pull/27448)
Based on #27447.

See https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html:
> Libc++ provides a debug mode that enables special debugging checks meant to detect incorrect usage of the standard library. These checks are disabled by default, but they can be enabled by vendors when building the library by using LIBCXX_ENABLE_DEBUG_MODE.

💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163046096)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"

The comments should be updated to avoid mentioning specific transaction states.

💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163045689)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"

This could also be used by `AbandonTransaction`.

💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163049972)
> Any reason to not enable assertions in the headers?

Given that this shouldn't conflict with also turning them on at compile time, i.e also as part of #27448, I don't think so.

Should we also do `_GLIBCXX_ASSERTIONS`?
> [Undefined by default. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.](https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html)

💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163056807)
_GLIBCXX_ASSERTIONS is enabled by _GLIBCXX_DEBUG

💬 theStack commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163054934)
nit: Is there any specific reason for this change? Without it, everything seems to work still fine on my side (but obviously, it shouldn't hurt to construct the message box with a parent...)

👍 theStack approved a pull request: "wallet, gui: bugfix, getAvailableBalance skips selected coins"
(https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1379656461)
ACK 68eed5df8656bed1be6526b014e58d3123102b03

Reviewed the changeset (with only light review for the qt unit tests, as I'm not too familiar in that area), verified manually that the [GUI coin selection issue](https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1253771673) is fixed, tested the tests by reverting the fix commits dc1cc1c35995dc09085b3d9270c445b7923fdb51 and cd98b717398f7b13ace91ea9efac9ce1e60b4d62 each and checked that the corresponding unit tests fail. LGTM.

💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163065163)
Yea I should have read another 2 lines down

💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163067961)
Added now.

💬 1440000bytes commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503775491)
Concept ACK

However I am not sure if other reviewers really matter here and only 2 main signet gatekeepers can approve this. To address the concern in https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1502605806 maybe this works only for custom signet.

How?

Maybe if bitcoin.conf has `signetchallenge=xxx` in it?

💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503777153)
> Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).

I don't know if that is a really fair criticism for signet, `-signetchallenge` is a consensus param as well.

> If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest conf
...

💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503780795)
> Maybe if bitcoin.conf has signetchallenge=xxx in it?

Yes, this currently it will throw an error if you don't have signet challenge set.

💬 jamesob commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1503832004)
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))

Reviewed code, built locally. Change looks good. Going to test with -asmap today.


<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`
...

👍 1440000bytes approved a pull request: "Allow configuirng target block time for a signet"
(https://github.com/bitcoin/bitcoin/pull/27446#pullrequestreview-1379793799)
utACK https://github.com/bitcoin/bitcoin/pull/27446/commits/15ca24bb49cd6a5027dcf57fc7b9bd1d33d367db

💬 earonesty commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503852579)
concept ack. signet is for tinkering

✅ achow101 closed an issue: "Selecting two coins will abort with "The amount exceeds your balance.""
(https://github.com/bitcoin-core/gui/issues/688)

🚀 achow101 merged a pull request: "wallet, gui: bugfix, getAvailableBalance skips selected coins"
(https://github.com/bitcoin/bitcoin/pull/26699)