📝 theuni opened a pull request: "RFC: Instanced logs"
(https://github.com/bitcoin/bitcoin/pull/30338)
PR'ing this early as an RFC because it's a lot of work that will require constant rebasing. This is the first logical step and is mostly mechanical.
This PR adds an "instance" param to each log function and callsite. As is, there should be no functional change here.
The intention is to allow for a specific instance to be passed in each time we use the logger, rather than the implicit global as we have it now. Eventually (one subsystem/chunk at a time), this will allow us to instantiate a p
...
(https://github.com/bitcoin/bitcoin/pull/30338)
PR'ing this early as an RFC because it's a lot of work that will require constant rebasing. This is the first logical step and is mostly mechanical.
This PR adds an "instance" param to each log function and callsite. As is, there should be no functional change here.
The intention is to allow for a specific instance to be passed in each time we use the logger, rather than the implicit global as we have it now. Eventually (one subsystem/chunk at a time), this will allow us to instantiate a p
...
💬 theuni commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189606984)
Ping @TheCharlatan @ajtowns @maflcko for Concept (N)ACKs
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189606984)
Ping @TheCharlatan @ajtowns @maflcko for Concept (N)ACKs
💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653288456)
re: https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652884524
I don't have a strong opinion, but I do think the NO_ARM commit 859e1c558fd9c8604fda361e465aa4eb4a676823 is a nice change because I don't think it fixes a purely cosmetic issue. I think it removes confusing behavior of opening a PR and seeing a job hang forever. The actual code change is very simple, just adding `skip: $NO_ARM == "true"` and I think the documentation change makes the whole "A self-hosted machine(s) can b
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653288456)
re: https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652884524
I don't have a strong opinion, but I do think the NO_ARM commit 859e1c558fd9c8604fda361e465aa4eb4a676823 is a nice change because I don't think it fixes a purely cosmetic issue. I think it removes confusing behavior of opening a PR and seeing a job hang forever. The actual code change is very simple, just adding `skip: $NO_ARM == "true"` and I think the documentation change makes the whole "A self-hosted machine(s) can b
...
💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653195226)
In commit "ci: forks can opt-out of CI branch push (Cirrus only)" (46be986c2cff004a081e89433715118637bb4eb3)
Maybe add `... as a custom env variable in Cirrus repository settings, accessible from https://cirrus-ci.com/github/my-organization/my-repository` because I don't think it is obvious where this can be set.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653195226)
In commit "ci: forks can opt-out of CI branch push (Cirrus only)" (46be986c2cff004a081e89433715118637bb4eb3)
Maybe add `... as a custom env variable in Cirrus repository settings, accessible from https://cirrus-ci.com/github/my-organization/my-repository` because I don't think it is obvious where this can be set.
👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139263214)
Code review ACK 0882babb102108a4351714cc3749fceba1ecbb35.
---
I think the current PR title / description are a little hard to understand. Maybe consider editing it to something like the following:
- Fix issues with CI on forks
I find myself making pull requests against my fork (mostly on top of [#28983](https://github.com/bitcoin/bitcoin/pull/28983), or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus task
...
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139263214)
Code review ACK 0882babb102108a4351714cc3749fceba1ecbb35.
---
I think the current PR title / description are a little hard to understand. Maybe consider editing it to something like the following:
- Fix issues with CI on forks
I find myself making pull requests against my fork (mostly on top of [#28983](https://github.com/bitcoin/bitcoin/pull/28983), or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus task
...
💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653307211)
Didn't verify, but could all of this be replaced by a call to `fs::path GetDefaultDataDir()`?
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653307211)
Didn't verify, but could all of this be replaced by a call to `fs::path GetDefaultDataDir()`?
👍 itornaza approved a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2139445312)
tested ACK 22fe97a5cc612f438ec6280757487934501bf527
Manually validated all instances of `testBlockValidity` in the entire code base are having their parameter list updated with `state` as their last output parameter using good old `git grep -n testBlockValidity`. Now, the interface can be used with the `libmultiprocess` code generator!
As for the standard testing, I got the pr from upstream, built and run all unit and functional tests including the entire extended test harness and all test
...
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2139445312)
tested ACK 22fe97a5cc612f438ec6280757487934501bf527
Manually validated all instances of `testBlockValidity` in the entire code base are having their parameter list updated with `state` as their last output parameter using good old `git grep -n testBlockValidity`. Now, the interface can be used with the `libmultiprocess` code generator!
As for the standard testing, I got the pr from upstream, built and run all unit and functional tests including the entire extended test harness and all test
...
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653310379)
I find this equally confusing, but was a bit reluctant to add a manual for Cirrus. Anyway, added.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653310379)
I find this equally confusing, but was a bit reluctant to add a manual for Cirrus. Anyway, added.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189651581)
@ryanofsky thanks for the PR description rewrite!
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189651581)
@ryanofsky thanks for the PR description rewrite!
💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653311450)
(It may also help with the Win CI errors)
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653311450)
(It may also help with the Win CI errors)
💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189661783)
@theuni I made a similar change in #29256. That approach might be less invasive because it makes the instance argument optional. So kernel code can be changed to log to a configurable instance while other code uses a global instance.
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189661783)
@theuni I made a similar change in #29256. That approach might be less invasive because it makes the instance argument optional. So kernel code can be changed to log to a configurable instance while other code uses a global instance.
👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139490748)
Code review ACK 576828e732bacb61b608cd89f669a2936d3e8d00.
I also tweaked formatting in the PR description to fix a few problems in the markdown copy / paste.
The suggested text "Fix issues with CI on forks" in the description was actually meant to be a replacement title for the PR, since the second commit in the PR fixes an issue with a github actions job, not a cirrus job. So could remove this and maybe update the title.
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139490748)
Code review ACK 576828e732bacb61b608cd89f669a2936d3e8d00.
I also tweaked formatting in the PR description to fix a few problems in the markdown copy / paste.
The suggested text "Fix issues with CI on forks" in the description was actually meant to be a replacement title for the PR, since the second commit in the PR fixes an issue with a github actions job, not a cirrus job. So could remove this and maybe update the title.
🤔 mzumsande reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2139518716)
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
I'd prefer combining some of the first commits with the respective test commits that actually make use of the change, but that's just my opinion ad maybe a matter of taste.
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2139518716)
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
I'd prefer combining some of the first commits with the respective test commits that actually make use of the change, but that's just my opinion ad maybe a matter of taste.
💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1653358992)
looks good to me now!
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1653358992)
looks good to me now!
👍 rkrux approved a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2139514584)
tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae)
`make, test/functional are successful`.
From what I understand about Silent Payments, the calculation of the recipient address is dependent on the input hash for which it makes sense to push down the `txoutputs` addition _after_ coin selection.
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2139514584)
tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae)
`make, test/functional are successful`.
From what I understand about Silent Payments, the calculation of the recipient address is dependent on the input hash for which it makes sense to push down the `txoutputs` addition _after_ coin selection.
💬 rkrux commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1653355957)
Although being a bit pedantic, but as this commit is supposed to be move-only, the removal of `CTxOut txout...` in line 1107 and the replacement of `push_back` with `emplace_back` in line 1115 could have been part of the previous commit to avoid any confusion for future readers.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1653355957)
Although being a bit pedantic, but as this commit is supposed to be move-only, the removal of `CTxOut txout...` in line 1107 and the replacement of `push_back` with `emplace_back` in line 1115 could have been part of the previous commit to avoid any confusion for future readers.
💬 romanz commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1653368609)
Thanks - fixed in 24e7925abb.
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1653368609)
Thanks - fixed in 24e7925abb.
💬 TheCharlatan commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2189768540)
Guix builds (aarch64):
```
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
4e3ea3b82
...
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2189768540)
Guix builds (aarch64):
```
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
4e3ea3b82
...
📝 brunoerg opened a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339)
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.
(https://github.com/bitcoin/bitcoin/pull/30339)
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.
💬 TheCharlatan commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2189810744)
Concept ACK
Re https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2090083022
> Doing that meaningfully means changing all the logging in all the bitcoin-kernel related modules (eg validation, mempool, blockstorage, scheduler, ...) to explicitly specify a logger rather than using the global functions, which seems like a lot of pointless busy work? I can't imagine why we'd want to support having multiple bitcoin-kernel instances in a single executable logging to different loggers; if
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2189810744)
Concept ACK
Re https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2090083022
> Doing that meaningfully means changing all the logging in all the bitcoin-kernel related modules (eg validation, mempool, blockstorage, scheduler, ...) to explicitly specify a logger rather than using the global functions, which seems like a lot of pointless busy work? I can't imagine why we'd want to support having multiple bitcoin-kernel instances in a single executable logging to different loggers; if
...