💬 glozow commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462497890)
> I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions.
It is helpful to know that the claim of simply not returning recent versions is false, thank you for looking into this.
> But someone who fights and throws accusations to keep their dnsseed in?
> Aggression about being included
> seed operators must have a good working relationship with the project and be supportive of its success.
I certainly agree that these are points to consider
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462497890)
> I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions.
It is helpful to know that the claim of simply not returning recent versions is false, thank you for looking into this.
> But someone who fights and throws accusations to keep their dnsseed in?
> Aggression about being included
> seed operators must have a good working relationship with the project and be supportive of its success.
I certainly agree that these are points to consider
...
💬 dergoegge commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3462509273)
> but it’s quite useful to be able to troubleshoot individual crashes/timeouts locally
This will still be possible, because you can build the `fuzz` binary without libFuzzer for reproduction purposes (this is what we do in the macOS fuzz CI job). Unless the bugs only manifest when running the fuzzer due to non-determinism.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3462509273)
> but it’s quite useful to be able to troubleshoot individual crashes/timeouts locally
This will still be possible, because you can build the `fuzz` binary without libFuzzer for reproduction purposes (this is what we do in the macOS fuzz CI job). Unless the bugs only manifest when running the fuzzer due to non-determinism.
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2474103060)
Thanks, taken
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2474103060)
Thanks, taken
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3462543390)
> this pull request conflicts with the following ones:
> https://github.com/bitcoin/bitcoin/pull/33629 (Cluster mempool by sdaftuar)
> https://github.com/bitcoin/bitcoin/pull/33591 (Cluster mempool followups by sdaftuar)
I think if you want to continue these refactors, it might be better if you picked code that doesn't conflict with Cluster Mempool or Kernel work, or things we are trying to ship in `v31`. Could instead do directories that rarely change, like `qt`, or `zmq`?
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3462543390)
> this pull request conflicts with the following ones:
> https://github.com/bitcoin/bitcoin/pull/33629 (Cluster mempool by sdaftuar)
> https://github.com/bitcoin/bitcoin/pull/33591 (Cluster mempool followups by sdaftuar)
I think if you want to continue these refactors, it might be better if you picked code that doesn't conflict with Cluster Mempool or Kernel work, or things we are trying to ship in `v31`. Could instead do directories that rarely change, like `qt`, or `zmq`?
👍 ryanofsky approved a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3268268351)
Code review ACK 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319, though I didn't look closely at the fuzz test. Sorry for the delay in reviewing this, I've been meaning to get to it for some time. I think all the changes seem good and look correct, but I did suggest a number of possible updates below, mostly simplifications that should make the PR smaller.
I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is
...
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3268268351)
Code review ACK 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319, though I didn't look closely at the fuzz test. Sorry for the delay in reviewing this, I've been meaning to get to it for some time. I think all the changes seem good and look correct, but I did suggest a number of possible updates below, mostly simplifications that should make the PR smaller.
I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293)
In commit "node/miner: allow generation of oversized templates" (bd9bed462e251a5d3bd6e376f25060aeba9539d4)
This ALLOW_OVERSIZED_BLOCKS tag approach seems reasonable, but not ideal because:
- It doesn't only allow larger blocks. It also stops enforcing coinbase sigops limits and stops enforcing minimum clamp values.
- It duplicates code between `BlockAssembler::BlockAssembler` constructors.
I think adding a new `BlockAssembler::Options` `bool allow_oversized_blocks{false}` option instea
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293)
In commit "node/miner: allow generation of oversized templates" (bd9bed462e251a5d3bd6e376f25060aeba9539d4)
This ALLOW_OVERSIZED_BLOCKS tag approach seems reasonable, but not ideal because:
- It doesn't only allow larger blocks. It also stops enforcing coinbase sigops limits and stops enforcing minimum clamp values.
- It duplicates code between `BlockAssembler::BlockAssembler` constructors.
I think adding a new `BlockAssembler::Options` `bool allow_oversized_blocks{false}` option instea
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472932269)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
I think it would probably be better for this to use unique_ptr instead shared_ptr (like fee_estimator and other members), so lifetime of the BlockTemplateCache is more explicit and predictable.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472932269)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
I think it would probably be better for this to use unique_ptr instead shared_ptr (like fee_estimator and other members), so lifetime of the BlockTemplateCache is more explicit and predictable.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473223014)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would seem nice to implement these comparisons in `operator==` helpers or other functions directly attached to BlockCreateOptions and `BlockAssembler::Options` structs. This way if new members are added to the structs, the comparison functions should not get out of date.
Otherwise it would be good to have comments in the two structs pointing out that this function may need to be updated if new fi
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473223014)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would seem nice to implement these comparisons in `operator==` helpers or other functions directly attached to BlockCreateOptions and `BlockAssembler::Options` structs. This way if new members are added to the structs, the comparison functions should not get out of date.
Otherwise it would be good to have comments in the two structs pointing out that this function may need to be updated if new fi
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473203366)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would be good to have a comment saying this is intentionally not comparing the `coinbase_output_script` field and why that is safe (assuming it is safe).
Also would be good to point out this is intentionally skipping `test_block_validity`.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473203366)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would be good to have a comment saying this is intentionally not comparing the `coinbase_output_script` field and why that is safe (assuming it is safe).
Also would be good to point out this is intentionally skipping `test_block_validity`.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473041954)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
It seems like there are a number of things here making this code more rigid and complicated than it needs to be which aren't explained. It seems like it would be more straightforward to:
- Make `BlockTemplateCache::GetBlockTemplate` method non-virtual
- Drop `MakeBlockTemplateCache` function, and just give `GetBlockTemplate` class a constructor.
- Drop `BlockTemplateCacheImpl` class and just im
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473041954)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
It seems like there are a number of things here making this code more rigid and complicated than it needs to be which aren't explained. It seems like it would be more straightforward to:
- Make `BlockTemplateCache::GetBlockTemplate` method non-virtual
- Drop `MakeBlockTemplateCache` function, and just give `GetBlockTemplate` class a constructor.
- Drop `BlockTemplateCacheImpl` class and just im
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473250518)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
`Template` name doesn't seem very descriptive. I'd consider calling it `CachedBlockTemplate` to be unambiguous, and also making it a top level struct instead of an inner struct so it is less entwined with the `BlockTemplateCacheImpl` class, and could be forward declared, accessed from tests, etc.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473250518)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
`Template` name doesn't seem very descriptive. I'd consider calling it `CachedBlockTemplate` to be unambiguous, and also making it a top level struct instead of an inner struct so it is less entwined with the `BlockTemplateCacheImpl` class, and could be forward declared, accessed from tests, etc.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473275740)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
This doesn't seem to be right because it is forward declaring a global `::BlockTemplateCache` class not in the `::node` namespace. Probably this should just be dropped.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473275740)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
This doesn't seem to be right because it is forward declaring a global `::BlockTemplateCache` class not in the `::node` namespace. Probably this should just be dropped.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472926339)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Not very important, but would be good to call UnregisterValidationInterface in Shutdown to unregister this. Otherwise, in theory if Init/Shutdown were called multiple times (e.g. in unit tests) there would be a memory leak. `node.block_template_cache` can be handled the same way as `node.fee_estimator` in `Shutdown`.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472926339)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Not very important, but would be good to call UnregisterValidationInterface in Shutdown to unregister this. Otherwise, in theory if Init/Shutdown were called multiple times (e.g. in unit tests) there would be a memory leak. `node.block_template_cache` can be handled the same way as `node.fee_estimator` in `Shutdown`.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473282308)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
`DEFAULT_CACHE_SIZE` is a very generic name for a global, maybe `DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE` would be better.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473282308)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
`DEFAULT_CACHE_SIZE` is a very generic name for a global, maybe `DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE` would be better.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2474162538)
Fixed the no-op issue where `getblockstats` was called twice and compared against itself. Now the RPC method is called once, and the test validates consistency between different query methods (hash vs height, single vs multiple stats) rather than checking if the method returns identical results to itself.
Also reverted changes that introduced the untested path problem.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2474162538)
Fixed the no-op issue where `getblockstats` was called twice and compared against itself. Now the RPC method is called once, and the test validates consistency between different query methods (hash vs height, single vs multiple stats) rather than checking if the method returns identical results to itself.
Also reverted changes that introduced the untested path problem.
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2474188422)
> include-what-you-use/boost-1.75-all.imp
I guess this is still correct-enough to use here, but are we making any effort to usptream updates, given it's 14 Boost releases out of date (current release is 1.89.0)? Seems possible that this could lead to incorrect IWYU suggestions, given any recent Boost refactorings?
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2474188422)
> include-what-you-use/boost-1.75-all.imp
I guess this is still correct-enough to use here, but are we making any effort to usptream updates, given it's 14 Boost releases out of date (current release is 1.89.0)? Seems possible that this could lead to incorrect IWYU suggestions, given any recent Boost refactorings?
💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462594921)
@maflcko ran configure again and here's the output https://gist.github.com/umrashrf/6a63476cce762e00a4f45603837c5bda
I just opened XCode and it asked me to install it. Still fails to build after installation.
New logs: https://gist.github.com/umrashrf/0714372957165e0264c8854d0060f660
XCode version: 26.0.1 (17A400)
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462594921)
@maflcko ran configure again and here's the output https://gist.github.com/umrashrf/6a63476cce762e00a4f45603837c5bda
I just opened XCode and it asked me to install it. Still fails to build after installation.
New logs: https://gist.github.com/umrashrf/0714372957165e0264c8854d0060f660
XCode version: 26.0.1 (17A400)
💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462647625)
Boost libs https://gist.github.com/umrashrf/15bfff6425025902a7f310d867929f14
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462647625)
Boost libs https://gist.github.com/umrashrf/15bfff6425025902a7f310d867929f14
🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555)
(https://github.com/bitcoin/bitcoin/pull/33555)
💬 ajtowns commented on issue "Batch tx broadcast RPC":
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3462702682)
I'd contend that if you could make a p2p connection, announce `INV tx1 tx2 ...` and reply to `GETDATA` requests (or BIP331 messages or whatever we add in future), and get your txs added to the node's mempool and relayed onwards, then you should be able to get the same result with a simple RPC call.
For errors, I think it would be preferable to have a parameter that decides whether the behaviour should be "only accept anything if you can accept everything" or "accept as much as you can and tell
...
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3462702682)
I'd contend that if you could make a p2p connection, announce `INV tx1 tx2 ...` and reply to `GETDATA` requests (or BIP331 messages or whatever we add in future), and get your txs added to the node's mempool and relayed onwards, then you should be able to get the same result with a simple RPC call.
For errors, I think it would be preferable to have a parameter that decides whether the behaviour should be "only accept anything if you can accept everything" or "accept as much as you can and tell
...