💬 furszy commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#discussion_r1251098387)
> It seems at least that we already have a couple of instances in master where both Qt::AlignLeading and Qt::AlignLeft are used together.

Quite sure that we have them because of QT creator setting the value automatically. Probably because there is some ancient qt version that doesn't have them as synonyms.

But for us, it should be fine to drop them, even qt 4.8 has them as synonyms ([link](https://het.as.utexas.edu/HET/Software/html/qt.html#AlignmentFlag-enum)).

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#discussion_r1251103633)
@furszy: Thanks a lot for checking. I removed the redundant `Qt::AlignLeading` properties from the second commit, i.e. it's only `Qt::AlignLeft` remaining.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1618871642)
Without going full cluster mempool, I think whatever linearization is given out, we should probably ensure that each remaining prefix of the ancestor package is an ancestor package itself, before attempting submission to the mempool. Skip the entry if it's not. Maybe with ancestor set scoring linearizer this is redundant? I can't tell. It's probably going to get a lot closer to the ordering you want regardless, as it catches things like https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1
...

💬 john-moffett commented on pull request "Exit and show error if unrecognized command line args are present":
(https://github.com/bitcoin-core/gui/pull/742#issuecomment-1618901090)
Modified to follow @hebasto 's idea of only allowing `bitcoin:` URIs if no other options follow.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1618905687)
@MarcoFalke @ryanofsky this is ready for review if you have time 🕺

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1618910236)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

👌

💬 instagibbs commented on pull request "[25.x] Parallel compact block downloads":
(https://github.com/bitcoin/bitcoin/pull/27752#issuecomment-1618917886)
utACK https://github.com/bitcoin/bitcoin/pull/27752/commits/b8ad3220a9068f10c2b3b14b40f211372aeece31

💬 instagibbs commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1618920692)
I've been running this for the last week, no transaction-based misbehavior reports in my logs. still unsure the juice is worth the squeeze, but appears to not be obviously harmful.

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1618940865)
Updates:
- Checking fuzz test failure

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251179626)
Ah drats, I did not realize that `CoinsResult.All()` produces a copy. Reverting this.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251181072)
I would prefer that the method `GetTotalBumpFee()` does not either return the sum of individual bump fees or the combined bump fee for the input set depending on when it is called. I’d be worried that it would be hard to understand later and might be a source of future bugs.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251181233)
Done

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1251211129)
ah great tip thanks ;-)

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1619034666)
> [64d0f23](https://github.com/bitcoin/bitcoin/commit/64d0f234e9aac490b03a5c019e86f1da56e62e01) looks good, except there is some messup in the contents of these two commits:

Thanks @vasild sorry about the messy rebase there. I cleaned up the commits and fixed the `const` nit. Rebased on master for merge conflicts.

💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1619047639)
tACK 5df988b534df842ddb658ad2a7ff0f12996c8478

Also tested backporting the tests. Trick is to disable the norm_pub check (or manually replacing `'` with `h`, changing the checksum where needed).

💬 ajtowns commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619112716)
> > I haven't come up with anything that seems like a good reason to vary consensus params though: if you want to test non-bitcoin behaviours, why modify bitcoin to do it?
>
> I just want to open a lightning channel and not need to wait an hour to use it...

With core lightning:

* on the "faucet" side, you need to create a zeroconf channel and fund it. to fund a 0.1 BTC channel with 0.05 BTC sent to the receiver, do: `lightning-cli -k --signet fundchannel id=$RECEIVER amount=1000000 fee
...

💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619120300)
@ajtowns I dont think we're gonna agree on this so whatever. I will continue to maintain this fork for its users.

https://github.com/benthecarman/bitcoin/releases/tag/custom-signet-blocktime

👍 jamesob approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1511690655)
ACK 03dfe04075668aa3b99a5d6d68adfcecffca0593 ([`jamesob/ackr/27746.3.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.3.sdaftuar.rework_validation_logic))

Reexamined commits locally. Built, tested each (or most). Examined [diff since
last review](https://gist.github.com/jamesob/8535a4b81fc29e890ce2eff9a0fb2f1d).

Changes include

- removing `BlockManager::AddDirty()`
- comment typo fixes
- AcceptBlock() under ChainMan (vs. standalone function)
- ad
...

💬 jamesob commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251222347)
4a9f0dae7ff64ba2c2f1eb1b44d0ca563726b776

Nit: feel like it may have been okay to leave in the previous assertion too (although this is sorted of indirectly tested by the `expected_assumed_valid` comparison). No need to change.

```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index a936a96577..f0ed54a521 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
...

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251261058)
Staring more at this, I am starting to wonder whether this would produce the same waste score for all possible BnB solutions, since BnB calculates the waste score with different parameters at the end of `SelectCoinsBnB()`. It does pass the coin selection tests. (I’ll let the fuzzer run over night to see whether that finds something else.)