💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2517091076)
For now I have kept a `try-catch block` and #33856 can be a potential solution. I will rebase when there's potential interest for the change. @TheCharlatan
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2517091076)
For now I have kept a `try-catch block` and #33856 can be a potential solution. I will rebase when there's potential interest for the change. @TheCharlatan
💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2517123474)
Is theere any rteason to keep this line?
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2517123474)
Is theere any rteason to keep this line?
💬 yuvicc commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#issuecomment-3520360967)
Code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
Logic correctly handles null pointers from empty spans.
(https://github.com/bitcoin/bitcoin/pull/33853#issuecomment-3520360967)
Code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
Logic correctly handles null pointers from empty spans.
💬 maflcko commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3520585217)
> Coverage before adding test: <img alt="before" width="2000" height="66" src="https://github.com/user-attachments/assets/28cb35b1-5127-4f82-ba74-5891bd36553c">
>
> Coverage after adding test: <img alt="after" width="2000" height="66" src="https://github.com/user-attachments/assets/41ebbd66-872e-4ba4-b550-84ff9bc61752">
I don't think those links work. Also, this is redundant to the corecheck result anyway?
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3520585217)
> Coverage before adding test: <img alt="before" width="2000" height="66" src="https://github.com/user-attachments/assets/28cb35b1-5127-4f82-ba74-5891bd36553c">
>
> Coverage after adding test: <img alt="after" width="2000" height="66" src="https://github.com/user-attachments/assets/41ebbd66-872e-4ba4-b550-84ff9bc61752">
I don't think those links work. Also, this is redundant to the corecheck result anyway?
👋 maflcko's pull request is ready for review: "ci: Enable experimental kernel stuff in most CI tasks"
(https://github.com/bitcoin/bitcoin/pull/33824)
(https://github.com/bitcoin/bitcoin/pull/33824)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909)
> we do getBlock and that provides everything we need
Ok, that's not really how you're supposed to use it :-)
It does make a breaking change potentially unpainful.
However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the `.capnp` method numbering. This could be worked around as follows:
```capnp
interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
destroy @0 (context :Proxy.Context) -> ();
getBlockHeader @1 (cont
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909)
> we do getBlock and that provides everything we need
Ok, that's not really how you're supposed to use it :-)
It does make a breaking change potentially unpainful.
However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the `.capnp` method numbering. This could be worked around as follows:
```capnp
interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
destroy @0 (context :Proxy.Context) -> ();
getBlockHeader @1 (cont
...
👍 TheCharlatan approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452188347)
Re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452188347)
Re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
🤔 hodlinator reviewed a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3452156315)
Concept ACK moving out-reference-parameters to return value
Not familiar with the kernel API but had a brief look at net_processing and validation.
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3452156315)
Concept ACK moving out-reference-parameters to return value
Not familiar with the kernel API but had a brief look at net_processing and validation.
💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517333716)
Should remove the shadowing `state` on line 4515 IMO.
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517333716)
Should remove the shadowing `state` on line 4515 IMO.
💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517330536)
Why not reuse `state` here?
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517330536)
Why not reuse `state` here?
💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517354777)
Worth adding an `Assume(!headers.empty())` before this, as `state` will be unset if we don't process any, whereas before we would return `true` regardless?
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517354777)
Worth adding an `Assume(!headers.empty())` before this, as `state` will be unset if we don't process any, whereas before we would return `true` regardless?
💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517387755)
I don't think there was ever a `signtransaction`, it was just a typo in the docs. The external signer feature was introduced in v22. The docs from that time mentions `signtransaction` and not `signtx`: https://github.com/bitcoin/bitcoin/blob/v22.0/doc/external-signer.md
Looking at the code from v22.0 however, the actual command was already `signtx`.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517387755)
I don't think there was ever a `signtransaction`, it was just a typo in the docs. The external signer feature was introduced in v22. The docs from that time mentions `signtransaction` and not `signtx`: https://github.com/bitcoin/bitcoin/blob/v22.0/doc/external-signer.md
Looking at the code from v22.0 however, the actual command was already `signtx`.
💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517395290)
`signtx` is not inherently stdin
All (required) commands should work with or without `--stdin`. Bitcoin Core currently only uses it for `signtx`, and that's worth mentioning, but it can change.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517395290)
`signtx` is not inherently stdin
All (required) commands should work with or without `--stdin`. Bitcoin Core currently only uses it for `signtx`, and that's worth mentioning, but it can change.
💬 hodlinator commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3520773269)
Thanks for trying this out @Eunovo!
> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.
Would it be acceptable to emit an error and halt, saying that `-reindex` for cases where nodes don't have sufficient block data also requires decreasing `-minimumchainwork=` to whatever level we have in the block data?
Could also explain that this is in order to keep logic consistent regardless of `-reindex`, and also say that running with lowered `-minimumch
...
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3520773269)
Thanks for trying this out @Eunovo!
> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.
Would it be acceptable to emit an error and halt, saying that `-reindex` for cases where nodes don't have sufficient block data also requires decreasing `-minimumchainwork=` to whatever level we have in the block data?
Could also explain that this is in order to keep logic consistent regardless of `-reindex`, and also say that running with lowered `-minimumch
...
💬 Sjors commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3520877571)
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
On macOS 26.0.1 I ran the full test suite with `-DENABLE_IPC` set to `OFF` and `ON`.
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3520877571)
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
On macOS 26.0.1 I ran the full test suite with `-DENABLE_IPC` set to `OFF` and `ON`.
💬 janb84 commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3520878892)
re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3520878892)
re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3520921384)
Here is my Guix build on RISC-V machine:
```
riscv64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635
...
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3520921384)
Here is my Guix build on RISC-V machine:
```
riscv64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635
...
🤔 frankomosh reviewed a pull request: "transaction: Adding script witness to ToString for CTxIn"
(https://github.com/bitcoin/bitcoin/pull/33711#pullrequestreview-3452460486)
I think the goal for this is reasonable, but doesn't it remove witness data from `CTransaction::ToString()` and break existing debug output?
(https://github.com/bitcoin/bitcoin/pull/33711#pullrequestreview-3452460486)
I think the goal for this is reasonable, but doesn't it remove witness data from `CTransaction::ToString()` and break existing debug output?
💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517605640)
Though I have not done a thorough test, I think that completely removing this might affect debug workflow. Before removing it, `Ctransaction::ToString()` is able to print all inputs, then a separate block with all witnesses, then all outputs. I don’t think this is possible anymore.
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517605640)
Though I have not done a thorough test, I think that completely removing this might affect debug workflow. Before removing it, `Ctransaction::ToString()` is able to print all inputs, then a separate block with all witnesses, then all outputs. I don’t think this is possible anymore.
💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517569031)
I think there might be an inconsistency in formatting here. `scriptWitness.ToString()` is called directly, while `scriptSig` uses `HexStr().substr(0, 24)`. Shouldn't there be some symmetry ?
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517569031)
I think there might be an inconsistency in formatting here. `scriptWitness.ToString()` is called directly, while `scriptSig` uses `HexStr().substr(0, 24)`. Shouldn't there be some symmetry ?