💬 brunoerg commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2209011214)
Rebased
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2209011214)
Rebased
💬 marcofleon commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665725484)
Is there a specifc reason for 100 here or is it not that important?
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665725484)
Is there a specifc reason for 100 here or is it not that important?
💬 brunoerg commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665725982)
Sure, I'll address it.
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665725982)
Sure, I'll address it.
💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209018643)
> it limits the number of connected stratum clients to 1, since there's only one ZMQ template feed (that seems ok for the standard suggested sv2 configurations)
I'm sure we could design the interface in a way that allows for more than one template config (if needed), e.g. the new rpc could accept a list of template configs and the zmq publisher publishes a template for each config.
> it precludes the ability to make the template provider public facing, even with an external tool
Why? In
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209018643)
> it limits the number of connected stratum clients to 1, since there's only one ZMQ template feed (that seems ok for the standard suggested sv2 configurations)
I'm sure we could design the interface in a way that allows for more than one template config (if needed), e.g. the new rpc could accept a list of template configs and the zmq publisher publishes a template for each config.
> it precludes the ability to make the template provider public facing, even with an external tool
Why? In
...
💬 brunoerg commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665727291)
I used the size we reserve in the RPC for this as reference.
(https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665727291)
I used the size we reserve in the RPC for this as reference.
💬 brunoerg commented on pull request "fuzz: fix key size in `crypter`":
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209030226)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665723276. Thanks, @marcofleon
(https://github.com/bitcoin/bitcoin/pull/30373#issuecomment-2209030226)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30373#discussion_r1665723276. Thanks, @marcofleon
💬 maflcko commented on pull request "refactor: use existing RNG object in ProcessGetBlockData":
(https://github.com/bitcoin/bitcoin/pull/30393#issuecomment-2209034642)
> There was some discussion on #29625 about reverting #28558 but this lgtm for now.
Happy to drop it, if the reason still applies, but I couldn't find one.
(https://github.com/bitcoin/bitcoin/pull/30393#issuecomment-2209034642)
> There was some discussion on #29625 about reverting #28558 but this lgtm for now.
Happy to drop it, if the reason still applies, but I couldn't find one.
👍 TheCharlatan approved a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2158973782)
ACK b9ba1a73094f4ad593b527e23d2681f41c225397
... but it would be nice to address the nits, will re-ACK quickly :)
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2158973782)
ACK b9ba1a73094f4ad593b527e23d2681f41c225397
... but it would be nice to address the nits, will re-ACK quickly :)
💬 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.