💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209097995)
> the new rpc could accept a list of template configs and the zmq publisher publishes a template for each config.
I suppose the external application could forward each to the right client.
> > it precludes the ability to make the template provider public facing, even with an external tool
>
> Why?
Because each consumer of this interface will have its own `coinbase_tx_additional_output_size`, which it can't communicate without RPC access. This can be avoided by picking a single size f
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209097995)
> the new rpc could accept a list of template configs and the zmq publisher publishes a template for each config.
I suppose the external application could forward each to the right client.
> > it precludes the ability to make the template provider public facing, even with an external tool
>
> Why?
Because each consumer of this interface will have its own `coinbase_tx_additional_output_size`, which it can't communicate without RPC access. This can be avoided by picking a single size f
...
💬 willcl-ark commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2209098078)
> There's a lot of stuff happening in one commit:
Right, I wanted to open this up specifically while WIP as I ended up less-sure about the idea of the change in general (nor the implementation approach). The final commit does indeed need breaking up, but I wanted to get feedback on the mechanics of the whole change before investing more time into it, so thank you very much for your detailed review!
> 1. behaviour change: allow removing a v2 connection when `!node_v2transport`
This was a
...
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2209098078)
> There's a lot of stuff happening in one commit:
Right, I wanted to open this up specifically while WIP as I ended up less-sure about the idea of the change in general (nor the implementation approach). The final commit does indeed need breaking up, but I wanted to get feedback on the mechanics of the whole change before investing more time into it, so thank you very much for your detailed review!
> 1. behaviour change: allow removing a v2 connection when `!node_v2transport`
This was a
...
🤔 marcofleon reviewed a pull request: "fuzz: fix key size in `crypter`"
(https://github.com/bitcoin/bitcoin/pull/30373#pullrequestreview-2159032605)
It still crashes with the same bad_alloc error. But changing this line
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/wallet/test/fuzz/crypter.cpp#L85
to `ConsumeRandomLengthByteVector(fuzzed_data_provider, 64)` fixes it for me. I thnk this is fine because `crypted_secret` ends up playing the same role as `cipher_text_ed` does in `Decrypt`.
(https://github.com/bitcoin/bitcoin/pull/30373#pullrequestreview-2159032605)
It still crashes with the same bad_alloc error. But changing this line
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/wallet/test/fuzz/crypter.cpp#L85
to `ConsumeRandomLengthByteVector(fuzzed_data_provider, 64)` fixes it for me. I thnk this is fine because `crypted_secret` ends up playing the same role as `cipher_text_ed` does in `Decrypt`.
🤔 furszy reviewed a pull request: "validation: sync chainstate to disk after syncing to tip"
(https://github.com/bitcoin/bitcoin/pull/15218#pullrequestreview-2159040159)
Code ACK 8887d28a014420668801d7e4c5d1bf45c5e93684
(https://github.com/bitcoin/bitcoin/pull/15218#pullrequestreview-2159040159)
Code ACK 8887d28a014420668801d7e4c5d1bf45c5e93684
💬 furszy commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1665775393)
Ok. np.
Still.. I know it is an overkill if we only introduce what I'm going to suggest for this PR but.. thought on adding a signal for the ibd completion state https://github.com/furszy/bitcoin-core/commit/85a050a8690e5431848658604e913c58ae45a4aa. It might be useful if we ever added other scenarios apart from this one.
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1665775393)
Ok. np.
Still.. I know it is an overkill if we only introduce what I'm going to suggest for this PR but.. thought on adding a signal for the ibd completion state https://github.com/furszy/bitcoin-core/commit/85a050a8690e5431848658604e913c58ae45a4aa. It might be useful if we ever added other scenarios apart from this one.
💬 brunoerg commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209139299)
> to ConsumeRandomLengthByteVector(fuzzed_data_provider, 64) fixes it for me. I thnk this is fine because crypted_secret ends up playing the same role as cipher_text_ed does in Decrypt.
Nice, I missed this but surely it's fine to limit it as well. Gonna change it.
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209139299)
> to ConsumeRandomLengthByteVector(fuzzed_data_provider, 64) fixes it for me. I thnk this is fine because crypted_secret ends up playing the same role as cipher_text_ed does in Decrypt.
Nice, I missed this but surely it's fine to limit it as well. Gonna change it.
💬 brunoerg commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209143368)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30373#pullrequestreview-2159032605
Now, we basically got rid of `ConsumeRandomLengthByteVector` with no specified `max_length`.
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209143368)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30373#pullrequestreview-2159032605
Now, we basically got rid of `ConsumeRandomLengthByteVector` with no specified `max_length`.
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272)
I think this test also actually resolves the TODOs that https://github.com/bitcoin/bitcoin/pull/29996/commits/556ff68cea1dfa6c444ef9c920dd0ce52ecc9668 aimed to resolve. I will elaborate more there.
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272)
I think this test also actually resolves the TODOs that https://github.com/bitcoin/bitcoin/pull/29996/commits/556ff68cea1dfa6c444ef9c920dd0ce52ecc9668 aimed to resolve. I will elaborate more there.
🤔 jonatack reviewed a pull request: "[WIP] net: return result from addnode RPC"
(https://github.com/bitcoin/bitcoin/pull/30381#pullrequestreview-2159102244)
Concept ACK, I'd find this useful.
(https://github.com/bitcoin/bitcoin/pull/30381#pullrequestreview-2159102244)
Concept ACK, I'd find this useful.
💬 jonatack commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665813596)
Suggest `Unable to open connection to <peer address>`
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665813596)
Suggest `Unable to open connection to <peer address>`
💬 jonatack commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665816378)
Agree with @stickies-v that returning the peer id would be more useful than the result.
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665816378)
Agree with @stickies-v that returning the peer id would be more useful than the result.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2209179140)
> ld64.lld: error: library not found for -lrt
Still at least one other bugfix to upstream here. ZMQ is trying to link against realtime for macOS.
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2209179140)
> ld64.lld: error: library not found for -lrt
Still at least one other bugfix to upstream here. ZMQ is trying to link against realtime for macOS.
💬 stickies-v commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2209184357)
> The rationale is directly from https://github.com/bitcoin/bitcoin/issues/20552
Sorry, I should have phrased this more clearly, the initial thoughts I mentioned were numbered to be addressing the same numbered elements from the paragraph above. So specifically the rationale for "behaviour change: allow removing a v2 connection when !node_v2transport", which I now understand is just to get it in line with the documentation.
> Do you have any suggestions on another way to determine success
...
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2209184357)
> The rationale is directly from https://github.com/bitcoin/bitcoin/issues/20552
Sorry, I should have phrased this more clearly, the initial thoughts I mentioned were numbered to be addressing the same numbered elements from the paragraph above. So specifically the rationale for "behaviour change: allow removing a v2 connection when !node_v2transport", which I now understand is just to get it in line with the documentation.
> Do you have any suggestions on another way to determine success
...
🤔 fjahr reviewed a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests"
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2159051199)
The first two commits look very good but I am not sure the third commit is still needed, but I am not completely sure yet and would like to hear what others think.
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2159051199)
The first two commits look very good but I am not sure the third commit is still needed, but I am not completely sure yet and would like to hear what others think.
💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665782174)
nit: `n3` is a pretty meaningless variable name that I wouldn't include in the logs. The comment above seems to be enough to explain what is going on so I think this can just be removed.
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665782174)
nit: `n3` is a pretty meaningless variable name that I wouldn't include in the logs. The comment above seems to be enough to explain what is going on so I think this can just be removed.
💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665838469)
This goes back to some of my earlier comments but I think it got lost because there were multiple things discussed at the same time: I think this is not the error we want to hit in this case (as I interpret the TODO) and I think we will not hit it in the future either, because once #30320 is merged this will hit the error message added there and not this one. This error message here mentions "work" but this is the actual work of the active chainstate, not some headers that the node has received.
...
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665838469)
This goes back to some of my earlier comments but I think it got lost because there were multiple things discussed at the same time: I think this is not the error we want to hit in this case (as I interpret the TODO) and I think we will not hit it in the future either, because once #30320 is merged this will hit the error message added there and not this one. This error message here mentions "work" but this is the actual work of the active chainstate, not some headers that the node has received.
...
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843)
It seems that `libbitcoin_consensus` still depends on `libbitcoin_util` via the `ParseHex()` call in `pubkey.cpp`.
However, the `"consensus util"` dependency is not allowed in `contrib/devtools/check-deps.sh`.
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843)
It seems that `libbitcoin_consensus` still depends on `libbitcoin_util` via the `ParseHex()` call in `pubkey.cpp`.
However, the `"consensus util"` dependency is not allowed in `contrib/devtools/check-deps.sh`.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209289584)
There's also the `RequestTransactionData(.Success)` message flow to consider. This message is separate from `NewTemplate` so that a miner can immediately start hashing while the back and forth the Job Declarator Server is happening.
In the approach where the Template Provider software only gets the details it needs to send a `NewTemplate` message, we'll need an additional RPC method of ZMQ feed to provide the information needed in a `RequestTransactionData.Success` message. In the approach w
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209289584)
There's also the `RequestTransactionData(.Success)` message flow to consider. This message is separate from `NewTemplate` so that a miner can immediately start hashing while the back and forth the Job Declarator Server is happening.
In the approach where the Template Provider software only gets the details it needs to send a `NewTemplate` message, we'll need an additional RPC method of ZMQ feed to provide the information needed in a `RequestTransactionData.Success` message. In the approach w
...
💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314790)
```
/src/bitcoin-core/depends/work/download/libevent-2.1.12-stable/libevent-2.1.12-stable.tar.gz.temp: OK
Extracting libevent...
/src/bitcoin-core/depends/sources/libevent-2.1.12-stable.tar.gz: OK
Preprocessing libevent...
patching file CMakeLists.txt
patching file cmake/AddEventLibrary.cmake
Configuring libevent...
-- The C compiler identification is Clang 18.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/local/bin
...
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314790)
```
/src/bitcoin-core/depends/work/download/libevent-2.1.12-stable/libevent-2.1.12-stable.tar.gz.temp: OK
Extracting libevent...
/src/bitcoin-core/depends/sources/libevent-2.1.12-stable.tar.gz: OK
Preprocessing libevent...
patching file CMakeLists.txt
patching file cmake/AddEventLibrary.cmake
Configuring libevent...
-- The C compiler identification is Clang 18.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/local/bin
...
💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314974)
https://github.com/google/oss-fuzz/actions/runs/9797043349/job/27052782296#step:7:9340
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314974)
https://github.com/google/oss-fuzz/actions/runs/9797043349/job/27052782296#step:7:9340