💬 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
...
💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705917794)
nit: AFAIR, for a long time we preferred to store the return value in a `ret` boolean and assert only on that (see e.g. `$ git grep ret.*secp256k1` vs `$ git grep assert.*secp256k1`), but not sure if we still have a developer guideline like "don't use assert with side-effects" in place (I think we once had, and removed it, so this way seems to be fine.)
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705917794)
nit: AFAIR, for a long time we preferred to store the return value in a `ret` boolean and assert only on that (see e.g. `$ git grep ret.*secp256k1` vs `$ git grep assert.*secp256k1`), but not sure if we still have a developer guideline like "don't use assert with side-effects" in place (I think we once had, and removed it, so this way seems to be fine.)
💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705843010)
nit, if you have to retouch:
```suggestion
CKey key = GenerateRandomKey();
```
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705843010)
nit, if you have to retouch:
```suggestion
CKey key = GenerateRandomKey();
```
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2271875972)
> This shouldn't send transactions without sending an INV. Instead, it should INV and wait for the GETDATA. (thanks to mzumsande for making a note of the behavior).
For context, an old discussion on this: https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1186579724 (I want to think a bit more on the potential benefits of #30572 before voicing an opinion).
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2271875972)
> This shouldn't send transactions without sending an INV. Instead, it should INV and wait for the GETDATA. (thanks to mzumsande for making a note of the behavior).
For context, an old discussion on this: https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1186579724 (I want to think a bit more on the potential benefits of #30572 before voicing an opinion).
👍 ryanofsky approved a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2221986325)
Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2221986325)
Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68
💬 sipa commented on issue "Unexpected behaviour when using `sortedmulti_a` descriptor":
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271899519)
@darosior I vaguely recall discussing this before, but I don't remember the reasoning. Imagine some particular application uses a specific fancy timelock/hashlock/2-of-3 combination that (when the script path is used) readily gives away it's a transaction by that particular application due to its unusual script. If the descriptor for that uses `multi_a(mobile_xpub,desktop_xpub,server_xpub)`, then the location of the key signed with will additionally publish which parties are involved on chain. U
...
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271899519)
@darosior I vaguely recall discussing this before, but I don't remember the reasoning. Imagine some particular application uses a specific fancy timelock/hashlock/2-of-3 combination that (when the script path is used) readily gives away it's a transaction by that particular application due to its unusual script. If the descriptor for that uses `multi_a(mobile_xpub,desktop_xpub,server_xpub)`, then the location of the key signed with will additionally publish which parties are involved on chain. U
...
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040)
Rebased due to the conflicts. More feedback has been addressed.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040)
Rebased due to the conflicts. More feedback has been addressed.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706004353)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706004353)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706005633)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706005633)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706006163)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706006163)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.
🚀 ryanofsky merged a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485)
(https://github.com/bitcoin/bitcoin/pull/30485)
👍 murchandamus approved a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2222087875)
tACK 6bfa26048dbafb91e9ca63ea8d3960271e798098
Reviewed code changes and resynced the node from scratch
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2222087875)
tACK 6bfa26048dbafb91e9ca63ea8d3960271e798098
Reviewed code changes and resynced the node from scratch
💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706016674)
Are we sure that we will drop support next release? Otherwise it might be better to be a bit less specific:
```suggestion
argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed in a future release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
```
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706016674)
Are we sure that we will drop support next release? Otherwise it might be better to be a bit less specific:
```suggestion
argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed in a future release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
```