💬 hebasto commented on pull request "doc: Drop no longer needed workaround for WSL":
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271614876)
> I suspect that condition still exists. Maybe it's safer to leave the command there until the configure step is removed by cmake?
I would be great to document such a condition more explicitly.
However, I'm okay with either outcome.
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271614876)
> I suspect that condition still exists. Maybe it's safer to leave the command there until the configure step is removed by cmake?
I would be great to document such a condition more explicitly.
However, I'm okay with either outcome.
💬 hebasto commented on pull request "doc: Drop no longer needed workaround for WSL":
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271624870)
> Maybe it's safer to leave the command there until the configure step is removed by cmake?
I've read https://github.com/bitcoin/bitcoin/issues/10269 again, and it seems all complains were about building a native package in depends, which is not longer the case.
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271624870)
> Maybe it's safer to leave the command there until the configure step is removed by cmake?
I've read https://github.com/bitcoin/bitcoin/issues/10269 again, and it seems all complains were about building a native package in depends, which is not longer the case.
💬 sipsorcery commented on pull request "doc: Drop no longer needed workaround for WSL":
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271636097)
> I've read #10269 again, and it seems all complaints were about building a native package in depends, which is no longer the case.
That does ring a bell.
utACK 16d82611812de4e91e7950fe6d31484cc7a9c937.
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271636097)
> I've read #10269 again, and it seems all complaints were about building a native package in depends, which is no longer the case.
That does ring a bell.
utACK 16d82611812de4e91e7950fe6d31484cc7a9c937.
💬 fanquake commented on pull request "doc: Drop no longer needed workaround for WSL":
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271641435)
> were about building a native package in depends, which is no longer the case.
Qt still builds a native package ?
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271641435)
> were about building a native package in depends, which is no longer the case.
Qt still builds a native package ?
💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1705793809)
Thanks. Will do (i.e. handle the errors together)
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1705793809)
Thanks. Will do (i.e. handle the errors together)
💬 hebasto commented on pull request "doc: Drop no longer needed workaround for WSL":
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271661544)
> > were about building a native package in depends, which is no longer the case.
>
> Qt still builds a native package ?
Right. It's a `qmake` itself. I meant the `native_*` packages that are handled by the depends build subsystem in its own way.
(https://github.com/bitcoin/bitcoin/pull/30597#issuecomment-2271661544)
> > were about building a native package in depends, which is no longer the case.
>
> Qt still builds a native package ?
Right. It's a `qmake` itself. I meant the `native_*` packages that are handled by the depends build subsystem in its own way.
🤔 glozow reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2221566752)
code review ACK. I haven't reviewed the fuzzers yet, doing that next. Tested by writing some benches that run a lot faster with the optimizations.
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2221566752)
code review ACK. I haven't reviewed the fuzzers yet, doing that next. Tested by writing some benches that run a lot faster with the optimizations.
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705696515)
Since we're subtracting both `base_iterations` (the N/4 estimate) and `iterations_done_now` from `iterations_left`, is the idea that `base_iterations` incorporates the computational overhead beyond the splitting operations?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705696515)
Since we're subtracting both `base_iterations` (the N/4 estimate) and `iterations_done_now` from `iterations_left`, is the idea that `base_iterations` incorporates the computational overhead beyond the splitting operations?
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705727595)
Wow, cool way to convey pair comparison
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705727595)
Wow, cool way to convey pair comparison
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705762087)
Would it make sense to use `if (best.feerate.IsEmpty() || best.feerate << m_depgraph.feerate(component)`? We set `imp` based on this afterward, and it seems like it could help it start smaller.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705762087)
Would it make sense to use `if (best.feerate.IsEmpty() || best.feerate << m_depgraph.feerate(component)`? We set `imp` based on this afterward, and it seems like it could help it start smaller.
🚀 fanquake merged a pull request: "build: Remove unused visibility checks"
(https://github.com/bitcoin/bitcoin/pull/30590)
(https://github.com/bitcoin/bitcoin/pull/30590)
💬 hebasto commented on pull request "build: Remove unused visibility checks":
(https://github.com/bitcoin/bitcoin/pull/30590#issuecomment-2271727382)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/307.
(https://github.com/bitcoin/bitcoin/pull/30590#issuecomment-2271727382)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/307.
💬 mzumsande commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1705871583)
> * I don't think this was intentional by the author of the pull request;
No, it wasn't intentional, I didn't consider the possibility of incorrect metadata - might simply use `snapshot_start_block->nHeight` instead of `base_blockheight` though I think?
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1705871583)
> * I don't think this was intentional by the author of the pull request;
No, it wasn't intentional, I didn't consider the possibility of incorrect metadata - might simply use `snapshot_start_block->nHeight` instead of `base_blockheight` though I think?
💬 mzumsande commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2271764814)
From the issue:
> The additional information can be helpful to us in certain failure cases, especially if we discuss issues like in #30288 seriously.
How? I could see how situations described in #30288 would require to add the block hash in a hypothetical scenario where we only saved the height - but we are in the reverse scenario where we already have the block hash, so I can't really think of a scenario where haveing the block height would help. I also didn't find any discussion about the
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2271764814)
From the issue:
> The additional information can be helpful to us in certain failure cases, especially if we discuss issues like in #30288 seriously.
How? I could see how situations described in #30288 would require to add the block hash in a hypothetical scenario where we only saved the height - but we are in the reverse scenario where we already have the block hash, so I can't really think of a scenario where haveing the block height would help. I also didn't find any discussion about the
...
💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2271781801)
> gradual increase of the discouragement score were non upgraded peers would be allow a number of score points before to be disconnected
That's like not-found, -- an idea that sounds attractive on it's face but on examination causes problems. For behavior that can't be prevented (like, say, inv-ing a transaction to you that already know), then it should obviously never disconnect for that. For behavior that can be prevented like sending a txn before an inv, sending an invalid block, etc..,
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2271781801)
> gradual increase of the discouragement score were non upgraded peers would be allow a number of score points before to be disconnected
That's like not-found, -- an idea that sounds attractive on it's face but on examination causes problems. For behavior that can't be prevented (like, say, inv-ing a transaction to you that already know), then it should obviously never disconnect for that. For behavior that can be prevented like sending a txn before an inv, sending an invalid block, etc..,
...
💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1705886931)
trailing whitespace, btw.
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1705886931)
trailing whitespace, btw.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705890739)
Indeed; largely due to the connected-component splitting. I've expanded the comments a bit.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705890739)
Indeed; largely due to the connected-component splitting. I've expanded the comments a bit.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705892110)
Perhaps, but due the BFS search order, the normal iteration code should very quickly try splitting the whole-component work items anyway. The goal here is really just making sure `best` is not empty, so branches aren't needed to deal with that case.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705892110)
Perhaps, but due the BFS search order, the normal iteration code should very quickly try splitting the whole-component work items anyway. The goal here is really just making sure `best` is not empty, so branches aren't needed to deal with that case.
👍 theStack approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2221804572)
Code-review ACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c
Left a few non-critical nits below, feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2221804572)
Code-review ACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c
Left a few non-critical nits below, feel free to ignore.
💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705899285)
I think this effectively only tests that given a certain private key, the same `XOnlyPubKey` objects result with both used methods (once via the secp256k1 keypair functions, once with `.GetPubKey()`). So strictly speaking computing and comparing the tweak hashes on top of that isn't needed and could be removed, but no strong feelings about that, as it also doesn't hurt. (Seems like this was already discussed in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1702774488 ff., so feel fre
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705899285)
I think this effectively only tests that given a certain private key, the same `XOnlyPubKey` objects result with both used methods (once via the secp256k1 keypair functions, once with `.GetPubKey()`). So strictly speaking computing and comparing the tweak hashes on top of that isn't needed and could be removed, but no strong feelings about that, as it also doesn't hurt. (Seems like this was already discussed in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1702774488 ff., so feel fre
...