💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2124941401)
Done

💬 achow101 commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2937298230)
> Are you sure? What are the exact steps to reproduce?

Nope.

Not sure what I did wrong yesterday, but tried again and it does seem to work.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2124994068)
This test with `now` is basically the same as `never`. Since the distinction is that `now` does actually rescan a little bit, it would be good to add in the setup for these tests a transaction that would be scanned by `now` but not `never`.

🤔 ismaelsadeeq reviewed a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2894302384)
> Moving away from leveldb opens the door towards doing this in parallel.

For this reason, I am Concept ACK.
No opinion on the approach yet; I am still studying the PR and prev discussion.

It might be a little too early for this but I'm excited and tried testing it out on Signet.

Just by building the branch and running the node on Signet, it crashes. See logs (I didnt start the node with `-reindex` option or maybe I might be doing something wrong though):
https://gist.github.com/is
...

🤔 fjahr reviewed a pull request: "tests: Expand HTTP coverage to assert libevent behavior"
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2894320133)
utACK f16c8c67bf137e64fbeea1242431baaa915a5c53

Looks good to me, I would consider my comments non-blocking and could potentially be addressed in a follow-up.

💬 fjahr commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125071947)
nit: I haven't looked at the actual data but this seems a bit fragile, if the second "result" is at the very end of res, the test would continue here but then fail below. Not sure how real that threat is but it could also be easily mitigated by doing one more `recv` I guess.

💬 fjahr commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2125070216)
nit: Could use a helper function to get rid of some duplication here (and even beyond the changes here across the whole test file).

```
def _get_http_connection(self, url_node):
conn = http.client.HTTPConnection(url_node.hostname, url_node.port)
conn.connect()
sock = conn.sock
return conn, sock
```

💬 tnndbtc commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2937789647)
@GregTonoski Just curious, how do you get the pruned node of 550MB? I'd like to explore a little more about this issue but searched on the internet and not able to find a easy solution to get a pruned node to start with.

👋 portlandhodl's pull request is ready for review: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)

💬 portlandhodl commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2938182776)
https://github.com/bitcoin/bitcoin/pull/27260/commits/2f2470e844c79695df0140103c73d742371554cc

This is the one! with this commit I have enabled multi network functional testing [Signet, Mainnet] this enables proof that under different networks the results will match expectations. This also had a few benefits too!

Previously commented out tests are now valid!
A lot of errors are now properly handed on a per network basis!

Thanks, and ready for review!

🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2894793634)
Thanks for the reviews!

Rebased 6645095e6ae0d9c208a47fc7322d859c4fe6afb0 -> 27c6576aba5c59402b8844703e04face2f41578c ([`pr/mine.23`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.23) -> [`pr/mine.24`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.23-rebase..pr/mine.24)) due to conflict with #31375, also bringing over some changes from #32297 and implementing some suggestions.

re: https://github.com/bitcoin/bi
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125412533)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112219834

> I attempted it in https://github.com/ismaelsadeeq/bitcoin/commit/b90c02ea22432c465c947937ba7c655db5d29bbc

Thanks, this seems like a possible direction to go in the future. But I could also see it going in a different direction of calling more mining functions and providing more complete functional test coverage of the mining interface.

I think the other direction might be more practically useful so don't want to
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125437820)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111949140

> IIRC we decided to not include spawning support just yet to keep it simple, so the comment may be outdated.

Yes the comment was just out of date and should be fixed now. Support for spawning was dropped in
https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2231676827 but implemented in the branch mentioned in the PR description: https://github.com/ryanofsky/bitcoin/commits/pr/mine-detach

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125432171)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111970868

> In "ipc: Add bitcoin-mine test program" [6841bae](https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8)
>
> The valid address probably shouldn’t be hardcoded here. It would be better to have a function that returns all valid addresses, with a default constant for unix.

This seems like a nice idea for a future change, but I don't think there is a very straightforward way to implement
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125446117)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112177316

> Should validate the address first?

This should not be necessary because connectAddress will validate it. The error string is in the exception printed below.

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125422549)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111981130

> Maybe specify the default datadir

This is a nice idea, and there is a GetDefaultDataDir() function that could potentially be called here. Only issue is this -datadir option documentation is copied from existing documentations and I think it is best to not introduce a discrepency. A PR to update all the -datadir options together though might be a good idea for a separate PR.

💬 ajtowns commented on pull request "init: deprecate `-blockmaxweight` startup option":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2938701306)
> > (TBH, I think these options should move into the template request, rather than being node startup options)
>
> Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.

> > I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight.
>
> That sounds like a good idea. Could you expand on that?

...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125788051)
I don't think this is right. This can lead to missing includes:

```
bash-5.2# cat map.imp
[
{ include: [ "<iterator>", public, "<utility>", public ] },
]
bash-5.2# cat repr.cpp
#include <utility>
#include <iterator>

void A(){
int a{};
int b{std::move(a)};
auto* it{&b};
std::advance(it,0);
}
bash-5.2# include-what-you-use -Xiwyu --mapping_file=map.imp repr.cpp

repr.cpp should add these lines:

repr.cpp should remove these lines:
- #include <iterator> // lines 2
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574)
I don't think this is right. Can't this lead to missing the tuple include when it is required in the future?

The correct fix is to just add the includes iwyu is asking for

💬 maflcko commented on pull request "build: add -Wthread-safety-pointer":
(https://github.com/bitcoin/bitcoin/pull/32647#issuecomment-2938885589)
lgtm ACK 83bfe1485c37d407de7eed11b8ad769a05f78b66