💬 rkrux commented on pull request "Add assumeutxo chainparams to release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1975597690)
> can be appended

can be or should be?

If it's important enough to be a part of the release process, might as well enforce it?
At least for mainnet, can be optional for test envs?

💬 placintaalexandru commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2690973097)
unfortunately I need libbitcoin_consensus and some headers from that lib

💬 darosior commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2690977098)
tACK 44041ae0eca9d2034b7c2bdef24724809cc35e90

💬 purpleKarrot commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2690996016)
Concept NACK. Please don't introduce hacks like this just to hide the underlying problems.

The necessity to nuke the build directory and do a clean configure should be considered a bug in the project. That bug should be identified and fixed instead of enforcing to nuke the build directory over and over during a rebase or after switching branches.

> Maybe one could have a `I_KNOW_WHAT_I_AM_DOING` environment variable ...

Working on multiple cmake projects in parallel, it would be nice if all p
...

💬 jonatack commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2691008230)
Concept ACK, will review after rebase.

💬 pinheadmz commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2691012709)
> unfortunately I need libbitcoin_consensus and some headers from that lib

Can you explain a bit more what you are working on?

⚠️ 0xB10C opened an issue: "Revisiting us self-hosting parts of our CI"
(https://github.com/bitcoin/bitcoin/issues/31965)
The project currently self-hosts the runners for ~half of our CI jobs. The other half of the CI jobs run on GitHub Actions hosted by GitHub. In an offline discussion, the question came up if we should move the self-hosted runners to some other non-self-hosted infrastructure or if there are other ways to improve on the following areas:

- reduce infrastructure maintenance burden: someone has to take care of the servers, but that's (normally) not too much work. Shouldn't be a single person maintai
...

💬 i-am-yuvi commented on pull request "rpc: add cli examples, update docs":
(https://github.com/bitcoin/bitcoin/pull/31958#issuecomment-2691134141)
ACK 52c66d9166b97f0a375d3accdd94eb5e9154862f

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2651321758)
Thanks for the review!

Updated 185c6a416df679ddc7b38750aa4d181c0b043d5b -> 623c0f0df878968dabbaa271a7654c638603f369 ([`pr/wrap.22`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.22) -> [`pr/wrap.23`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.22..pr/wrap.23)) cleaning up comments and help with suggestions

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1975744016)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r196352313

> nit: unused include

Thanks, fixed!

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1975744225)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963537775

> `tx` is missing and `test` is part of the internal commands, so:

Thanks, fixed!

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1975745012)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963378155

Thanks, fixed!

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1975744363)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963540791

> Some leftovers, I guess from previous incarnation of this which did not use `R"(...)"`:

Thanks, fixed!

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1975744590)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963558283

> This prints "bitcoin gui". One might get the impression that the command is "bitcoin gui", whereas it is just "gui".

Thanks for pointing that out. Changed to just print the command like git.

💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1975762490)
It is my understanding that the `feature_block.py` test suites turns on [`-acceptnonstdtxn=1`](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/feature_block.py#L90) which turns off this policy check. Thus we don't receive any error message when submitting the transaction to the mempool.

https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/validation.cpp#L886

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2691174968)
> Concept ACK. Reviewed as part of #31375, but did not check if they're identical.

Thanks I should probably just rearrange the commits in the other PR now that this PR was created, but they are identical and this can be checked with:

```shell
$ git range-diff origin/pull/31866/head~2..origin/pull/31866/head origin/pull/31375/head~5..origin/pull/31375/head~3
1: e60ff180c29d = 1: a92ecaba9521 test, refactor: Add TestNode.binaries to hold binary paths
2: 8f5228d92394 = 2: 6c457f40436a
...

💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975780370)
Kept the base58 fuzz changes only, [removed the base[32/64] changes](https://github.com/bitcoin/bitcoin/compare/a63c5c1a8a012b694b8e260fd5d2fda3c8785e98..1e4e183acbb1dd9b67f761b1eef2b9b200b3f6ab)

👍 ryanofsky approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2650921706)
Code review ACK ccc34a26170ef8c63a6f211eda4464e954873e89. Since last review just added a commit replacing IsArgSet call. I left some suggestions that could be followed up, but they are not very important.

💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975653432)
re: https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966627276

> If you insist on keeping structs, maybe hoist up the braces to conform with the dev-notes?

Not important but do agree it would be good to use recommended style for structs with opening brace on the same line (can just run `clang-format`).

Also agree classes make more sense than structs here. Compilers may not care about the difference but they mean different things to developers. Structs usually provide data defi
...

💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975511515)
In commit "cli, refactor: for -getinfo, replace IsArgSet() with GetBoolArg()" (ccc34a26170ef8c63a6f211eda4464e954873e89)

Would be good to drop "refactor" from commit subject since this is a small change in behavior, not a pure refactoring. It be described as a bugfix: no longer treating `-nogetinfo` and `-getinfo=0` the same as `-getinfo` and `-getinfo=1`, instead treating them like getinfo was not specified.