💬 JeremyRubin commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2561531373)
approach ack on removing libevent and having it operate over unix socket -- http functionality / REST can be provided via a proxy server.

💬 davidgumberg commented on pull request "rpc: Extend scope of validation mutex in generateblock":
(https://github.com/bitcoin/bitcoin/pull/31563#issuecomment-2561605494)
ACK https://github.com/bitcoin/bitcoin/pull/31563/commits/fa62c8b1f04a5386ffa171aeff713d55bd874cbe

I checked the [two](https://github.com/bitcoin/bitcoin/blob/fc7b21484703da606c5c69b23daee8c39506d90c/src/node/miner.cpp#L171-L173) [other](https://github.com/bitcoin/bitcoin/blob/fc7b21484703da606c5c69b23daee8c39506d90c/src/rpc/mining.cpp#L713-L718) users of `TestBlockValidity()` and both seem to not have this issue.

Given that `TestBlockValidity()`'s is only used in the RPC interface, and ca
...

💬 davidgumberg commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1897104374)
feel-free-to-disregard: Is there a reason to make `$CI_BUILD_MOUNT` inconsistent (including `--mount`) with the other mount vars?

💬 davidgumberg commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1897115704)
feel-free-to-disregard-followup: After thinking more about my comment below, (https://github.com/bitcoin/bitcoin/pull/31428/files#r1897104374) It is because we might not use *any* volume mount for the build dir, and we want to be able to leave it blank here.

But I'm wondering why the build directory doesn't get it's own volume, why doesn't the same reasoning from #27033 apply here?

💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1897174064)
Yes, it is not possible without major changes, see https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893812448

💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1897175352)
> But I'm wondering why the build directory doesn't get it's own volume, why doesn't the same reasoning from #27033 apply here?

The build output is not really a cache for the next build, but rather the result of the build, so a volume doesn't help. In fact it can only be overwritten by `DANGER_CI_ON_HOST_FOLDERS`, where the goal is to use a folder, not a volume.

💬 maflcko commented on pull request "test: add unit tests for SigningResultString function":
(https://github.com/bitcoin/bitcoin/pull/31567#issuecomment-2561703716)
No objection, but I also don't see the point of adding a unit test for a trivial function that stringifies an enum class. Also, I am not sure about adding 3 lines of test code with a pull request description that is AI generated and mostly just repeats the changes in words.

Adding test coverage is great, but I think here it would be better to add it as an RPC test, so that it is clear how to reach each code path from real code, by a real user. See the RPC coverage: https://maflcko.github.io/b
...

💬 maflcko commented on pull request "rpc: Extend scope of validation mutex in generateblock":
(https://github.com/bitcoin/bitcoin/pull/31563#issuecomment-2561750873)
> Given that `TestBlockValidity()`'s is only used in the RPC interface, and can't guarantee that it's caller won't give it a bad `pindexPrev` argument (#6123 also) , would it make sense to demote the assertion to an `Assume()` or to `return false`?

No, it is not possible to replace `Assert` with `Assume` here. After a "failed" assume after an internal runtime error (coding error), in non-debug builds, the program execution will continue and then fail later-on with another error (possibly givi
...

💬 maflcko commented on pull request "rpc: Extend scope of validation mutex in generateblock":
(https://github.com/bitcoin/bitcoin/pull/31563#issuecomment-2561751297)
> Added the following test to `rpc_generate.py`:

Nice, pushed something based on your idea!

💬 brunoerg commented on pull request "test: add unit tests for SigningResultString function":
(https://github.com/bitcoin/bitcoin/pull/31567#issuecomment-2561766726)
Concept ~0. I agree with @maflcko, I prefer to have it as an RPC test, I don't see a strong value on having a unit test for this trivial thing.

✅ laanwj closed an issue: "Please provide 32 bits builds again"
(https://github.com/bitcoin/bitcoin/issues/31557)

💬 laanwj commented on issue "Please provide 32 bits builds again":
(https://github.com/bitcoin/bitcoin/issues/31557#issuecomment-2561823010)
> Installing a 32-bit system Linux and compiling yourself should be easier, but I haven't tried it.

FWIW, a year or two ago i compiled bitcoin core on a 2009 32-bit Acer Aspire One running Debian linux. It could sync the chain though it obviously took weeks.

This was just an experiment of course. x86 32-bit is a dead platform, it makes no sense to bring it back as a release build.

This is basically an individual help request. i'd suggest finding some 64-bit machine (even the crappiest o
...

💬 lucasbalieiro commented on pull request "test: add unit tests for SigningResultString function":
(https://github.com/bitcoin/bitcoin/pull/31567#issuecomment-2561845324)
@maflcko and @brunoerg , agreed. Thanks for the time. Closing the PR.

✅ lucasbalieiro closed a pull request: "test: add unit tests for SigningResultString function"
(https://github.com/bitcoin/bitcoin/pull/31567)

💬 romanz commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#discussion_r1897341483)
Is the `import` expected to fail on CI?
If so, I am not sure that the test below can run successfully if `sqlite3` is not available...

🤔 fjahr reviewed a pull request: "validation: Send correct notification during snapshot completion"
(https://github.com/bitcoin/bitcoin/pull/31556#pullrequestreview-2522565392)
tACK ca0753160a8ebc113e08d62c7b6cbe8fa98455e6

Good catch, feel free to ignore my comments unless you have to retouch.

💬 fjahr commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1897340026)
nit: I prefer to not log the prep work of these tests, only what describes the actual point of the test either at the start or at the check. (It can still be a comment though)

I see that you follow roughly the same style of the logging in the rest of the file but the actually interesting part that sets this test apart from the other tests is in the comment. The logs are just printing rather generic stuff. So I basically revert the logging ad comments in this block. I.e. L201 becomes a log and
...

💬 fjahr commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1897367226)
nit: could have been `const`

💬 fjahr commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1897370877)
I like to see `ensure_for` getting used but I think I would have opted for `self.wait_until(lambda: (n2.getbalance() == 34), timeout=5)` or so here. Is there a case I am missing where the balance might change after having been 34 that you are covering with this?

💬 fjahr commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1897368195)
nit: newline not necessary imo