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

🤔 Xekyo reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1511646387)
Addressed comments by @S3RK and @achow101

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251210983)
Used 3×target_fee_rate instead

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251243230)
I’m not sure I see a big difference between setting up the wallet with a confirmed UTXO from which I send an unconfirmed transaction with a varying feerate, or setting up the wallet to have an unconfirmed UTXO with varying feerate. The former may have the advantage that when I need multiple UTXOs, it’s easier to understand what’s different.

Currently expecting to keep it this way

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251248054)
Thanks, that check doesn’t make sense, I’ve removed it.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251192852)
Thanks for catching this. Fixed.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251203025)
Moved all comments about what the tests do to the implementations of the tests

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251204807)
Yeah, you’re right. Removed duplicate check.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251230645)
Oops, I only fixed the rename in the next commit. Fixing.

🤔 mzumsande reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1511762976)
Code Review ACK 03dfe04075668aa3b99a5d6d68adfcecffca0593

I reviewed the code again (didn't look that deep into the test changes), and did some manual testing.

> Not sure what's up with the arm failure, but tests pass for me locally.

Pretty sure it's unrelated, same thing just happened on master: https://github.com/bitcoin/bitcoin/runs/14733904119 (some discussion in #27988)

💬 mzumsande commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251271660)
Now `ChainstateManager` has `ActiveSnapshotBase()` and `GetSnapshotBaseBlock()` which, as far as I understand it, do the same thing except for the caching.
Would it have been possible to add the caching to the existing `GetSnapshotBaseBlock()` instead?

💬 ajtowns commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619186257)
> @ajtowns I dont think we're gonna agree on this so whatever.

I don't understand why you're ignoring an approach that seems to do a better job of getting what you want (instant channels, without even having to wait a minute), and can work on mainnet (so won't end up developing a UX that's only conceivably good for demos and doesn't work with real money), and is already supported by CLN, LND and LDK. If you don't want to explain, or even just figure "if it's dumb but it works that's good eno
...