💬 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)

💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163166007)
It was because I was accessing the parent to click on the message box from the unit test (the dialog `exec()` method blocks the thread until the user presses any of the buttons). And, later on, changed my mind and accessed it via the `QApplication::topLevelWidgets` call.

It doesn't hurt to have it. This would had been a leak if the message box would had been constructed via a `new` call.

💬 furszy commented on issue "Unable to create PSBT for legacy watchonly wallets in the GUI":
(https://github.com/bitcoin/bitcoin/issues/26687#issuecomment-1503887860)
https://github.com/bitcoin/bitcoin/pull/26699 merged, can be closed.

💬 sdaftuar commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1162786364)
Not sure if I'm just reading too fast, but was the intent here to check that the mempool minfee for the *child* is <= 800? Seems like we did the parent checks up above, but maybe I'm missing something...?

✅ fanquake closed an issue: "Unable to create PSBT for legacy watchonly wallets in the GUI"
(https://github.com/bitcoin/bitcoin/issues/26687)

💬 ishaanam 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_r1163192633)
Done