💬 hodlinator commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1945468203)
I ran the *test/lint/test_runner/* runner (`COMMIT_RANGE="HEAD~3..HEAD" cargo run`) on my suggestion (which runs *ruff* if it's installed). It did not emit any errors, so I think you should be fine.

Couldn't find the precise rule against my suggestion, but Q003 seems like it would be in favor of it: https://docs.astral.sh/ruff/rules/#flake8-pytest-style-pt

💬 Randy808 commented on issue "nSequence is not set when spending from satisfiable descriptor with relative timelock":
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2641087658)
@sipa My original usecase was a decaying multisig using `thresh` like your "A 3-of-3 that turns into a 2-of-3 after 90 days" example on https://bitcoin.sipa.be/miniscript/ except I was doing a 2-of-2 that turned into a 1-of-2. In this case my wallet would have one of the private keys available at the time of spending with the `older` clause met. Also, to re-iterate, I can still spend the utxo if I construct the transaction using `createrawtransaction`, `signrawtransactionwithwallet`, and `sendra
...

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2641101801)
> When would we remove these symlinks? This seems like overkill, and just deferring breakage until later?

> We've not really attempted any autotools compatibility

I don't see any reason to delete the symlinks in the future. And they are not created for compatibility with autotools but for compatibility with cmake defaults and compatibility with our own build.

Also, if we did decide to delete the symlinks in the future it would not be deferring the breakage but replacing **silent breaka
...

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1945504125)
What problem do you see this solving? Creating a symlink pointing at a target that does not exist should be fine. It will still point to the right place.

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2641120104)
> Did anyone test this on Windows?

Of course would be nice to test this on windows but it should work there according to cmake release notes: https://cmake.org/cmake/help/v3.13/release/3.13.html?highlight=release%20notes#command-line

💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2641150108)
Converting to a draft after today's offline discussion with @theuni.

📝 hebasto converted_to_draft a pull request: "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows"
(https://github.com/bitcoin/bitcoin/pull/31158)
On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` fails:
```
> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
> cmake --build build --config Release -t bitcoin-chainstate
...
LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]
```

This PR
...

🤔 fjahr reviewed a pull request: "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds"
(https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2600123724)
Concept ACK

💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1945553930)
In `gen-manpages.py` and `gen-bitcoin-conf.sh` we use `BUILDDIR` fwiw.

💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1945549602)
I agree, hardcoding a different path isn't much of a fix even if it's the one we use in all our examples. Better to have a way to pass the path in as a parameter.

💬 Willrok91 commented on issue "Generalized fee bumping":
(https://github.com/bitcoin-core/gui/issues/822#issuecomment-2641233162)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7

💬 Willrok91 commented on issue "Send: ability to (re)view automatically selected coins ":
(https://github.com/bitcoin-core/gui/issues/778#issuecomment-2641236859)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2641303013)
> It might be good to also adjust the title of this test for CoinGrinder to "3) Test selection when some selections with sufficient funds would exceed the maximum allowed weight"

I find the use of "selection" twice in the sentence doesn't add much clarity. Now with the added assertions, I feel like the tittle fits the test however I'm open to a better title.

> Not really. The weight-optimality of CoinGrinder solutions is shown per the fuzz target coin_grinder_is_optimal.

Interesting.
...

💬 jarolrod commented on pull request "Prepare "Open Transifex translations for v29.0" release step":
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2641784769)
post merge ack

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945972101)
Done.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945976586)
No, we do not want to remove entries from `m_listen_permissions` or from `m_listen` when disconnecting a single peer. The entries in those two maps are per listening address. E.g. if we listen on `1.1.1.1:8333` and designate that every peer that connects to that address gets `permissions1` and listen on `2.2.2.2:8333` and peers that connect to that address get `permissions2`. This stays unchanged as peers connect and disconnect. We would want to remove entries from those maps if we stop listenin
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945983707)
`addr` is our listening address. We can't listen on the same address two times:

If I put this in my `bitcoind.conf`:
```
bind=127.0.0.1:20001
bind=127.0.0.1:20002
bind=127.0.0.1:20003
bind=127.0.0.1:20003
```

I get:

```
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20001
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20002
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20003
2025-02-07T05:39:56Z [net:error] Cannot
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945989789)
The proposed snippet is more elegant and performant, I like it. It relies, however, on `None` being `0` and will break in a subtle way if that is changed. I will leave it as it is. This is executed once per newly accepted connection which is not super-often and the performance gain would be negligible.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945991471)
Agree, will leave it as it is.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946000478)
This method was only dealing with listening sockets, so the name `CloseSockets()` with a comment "Close all sockets" was confusing. Renamed to `StopListening()`.