💬 delta1 commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656383645)
ACK https://github.com/bitcoin/bitcoin/commit/033acdf03da4a77d69fb58f7cab97dd1073fb83e
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656383645)
ACK https://github.com/bitcoin/bitcoin/commit/033acdf03da4a77d69fb58f7cab97dd1073fb83e
🚀 fanquake merged a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31855)
(https://github.com/bitcoin/bitcoin/pull/31855)
💬 delta1 commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656392782)
similar instance on line 94 now that i see it
https://github.com/bitcoin/bitcoin/pull/31855/files#diff-91d294c18d2f63ceb1c9aadff3b3ae046b252b763d69b13f930f7de7b399f440R94
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656392782)
similar instance on line 94 now that i see it
https://github.com/bitcoin/bitcoin/pull/31855/files#diff-91d294c18d2f63ceb1c9aadff3b3ae046b252b763d69b13f930f7de7b399f440R94
⚠️ jirijakes opened an issue: "doc/zmq: Note about endianness does not match reality"
(https://github.com/bitcoin/bitcoin/issues/31856)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
PR #23471 added a note to ZMQ's documentation page saying that:
> […] 32-byte hashes are in Little Endian and not in the Big Endian format that the RPC interface and block explorers use to display transaction and block hashes.
Also:
> `| hashtx | <32-byte transaction hash in Little Endian> | <uint32 sequence number in Little Endian>`
> `| hashblock | <32-byte block hash in Little Endian
...
(https://github.com/bitcoin/bitcoin/issues/31856)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
PR #23471 added a note to ZMQ's documentation page saying that:
> […] 32-byte hashes are in Little Endian and not in the Big Endian format that the RPC interface and block explorers use to display transaction and block hashes.
Also:
> `| hashtx | <32-byte transaction hash in Little Endian> | <uint32 sequence number in Little Endian>`
> `| hashblock | <32-byte block hash in Little Endian
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954403094)
I'm not expert in this area to decide if this is to sterile or not (i.e. testing a too narrow slice that isn't representative, where the biggest speedup doesn't cause any measurable macro change), will let others decide if this is too `micro` or not.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954403094)
I'm not expert in this area to decide if this is to sterile or not (i.e. testing a too narrow slice that isn't representative, where the biggest speedup doesn't cause any measurable macro change), will let others decide if this is too `micro` or not.
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954418526)
is the` AddToBlockIndex` cheap enough to be done inside the benchmark
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954418526)
is the` AddToBlockIndex` cheap enough to be done inside the benchmark
🤔 rkrux reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2614799077)
ACK af76664b12d8611b606a7e755a103a20542ee539
I reviewed range-diff:
```
git range-diff d5e28457a099cd546e757984043f28ba9f6689b1...af76664b12d8611b606a7e755a103a20542ee539
```
Newer changes are incorporating previous suggestions such as fixing comments, adding couple more assertions in the functional tests, getting rid of unused private key in test. Clarified my previous suggestion.
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2614799077)
ACK af76664b12d8611b606a7e755a103a20542ee539
I reviewed range-diff:
```
git range-diff d5e28457a099cd546e757984043f28ba9f6689b1...af76664b12d8611b606a7e755a103a20542ee539
```
Newer changes are incorporating previous suggestions such as fixing comments, adding couple more assertions in the functional tests, getting rid of unused private key in test. Clarified my previous suggestion.
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954387143)
Not sure if I was clear earlier. I am suggesting this change:
```
some_keys_addr = self.old_node.deriveaddresses(some_keys_priv_desc)[0]
all_keys_addr = self.old_node.deriveaddresses(all_keys_priv_desc)[0]
```
Reason being that these are imported in the wallet down below, a wallet that has been created on old_node. Same reasoning for the suggestion in the next comment for the `test_taproot` test.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954387143)
Not sure if I was clear earlier. I am suggesting this change:
```
some_keys_addr = self.old_node.deriveaddresses(some_keys_priv_desc)[0]
all_keys_addr = self.old_node.deriveaddresses(all_keys_priv_desc)[0]
```
Reason being that these are imported in the wallet down below, a wallet that has been created on old_node. Same reasoning for the suggestion in the next comment for the `test_taproot` test.
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954390887)
Aah right, thanks.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954390887)
Aah right, thanks.
💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136)
1. Rebased due to the conflict with the merged bitcoin/bitcoin#30911.
2. Addressed the recent @theuni's feedback.
3. Fixed minor bugs, such as: https://github.com/bitcoin/bitcoin/blob/18cc0a55595f9dc1f2e561743201a05754996b64/cmake/module/TargetDataSources.cmake#L6
> Also, since you're messing with this, please consider the `CODEGEN` option for `add_custom_command`, which would need feature-gating same as `DEPENDS_EXPLICIT_OPT`.
Nice! Added.
Please note that `cmake_policy(SET CMP0171 N
...
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136)
1. Rebased due to the conflict with the merged bitcoin/bitcoin#30911.
2. Addressed the recent @theuni's feedback.
3. Fixed minor bugs, such as: https://github.com/bitcoin/bitcoin/blob/18cc0a55595f9dc1f2e561743201a05754996b64/cmake/module/TargetDataSources.cmake#L6
> Also, since you're messing with this, please consider the `CODEGEN` option for `add_custom_command`, which would need feature-gating same as `DEPENDS_EXPLICIT_OPT`.
Nice! Added.
Please note that `cmake_policy(SET CMP0171 N
...
💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954428888)
Thanks! [Rebased](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954428888)
Thanks! [Rebased](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).
💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954429250)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954429250)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).
💬 maflcko commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2656491879)
CI passed and coverage is as expected:
* This pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/faad87189015e559/733f70b21be22925/fuzz.coverage/index.html
* Before this pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/6a46be75c43e6489/733f70b21be22925/fuzz.coverage/index.html
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2656491879)
CI passed and coverage is as expected:
* This pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/faad87189015e559/733f70b21be22925/fuzz.coverage/index.html
* Before this pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/6a46be75c43e6489/733f70b21be22925/fuzz.coverage/index.html
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954433827)
I pushed a commit to explain the long poll behaviour. Also added an early return above this point, just for clarity.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954433827)
I pushed a commit to explain the long poll behaviour. Also added an early return above this point, just for clarity.
💬 ryanofsky commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954434025)
re: https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137
> Nit in [a8fedb3](https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62): Why is this needed? It seems like a no-op:
Right it is is a no-op, see #30529 which makes the change you suggested and removes many other misuses of IsArgSet
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954434025)
re: https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137
> Nit in [a8fedb3](https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62): Why is this needed? It seems like a no-op:
Right it is is a no-op, see #30529 which makes the change you suggested and removes many other misuses of IsArgSet
💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656503405)
- Removed the nested loop in `GetAdaptersAddresses` (@sipa)
- Made doc comment for `FromSockAddr` more concise (@davidgumberg)
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656503405)
- Removed the nested loop in `GetAdaptersAddresses` (@sipa)
- Made doc comment for `FromSockAddr` more concise (@davidgumberg)
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2656504648)
I pushed a commit to clarify long poll behavior.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2656504648)
I pushed a commit to clarify long poll behavior.
🤔 l0rinc reviewed a pull request: "Benchmark Chainstate::ConnectBlock duration"
(https://github.com/bitcoin/bitcoin/pull/31689#pullrequestreview-2614869602)
Concept ACK, please see my remaining comments:
<details>
<summary>Details</summary>
```patch
Index: src/bench/connectblock.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp
--- a/src/bench/connectblock.cpp (revision b3a64758bc09eb60858c23292ba7f1b9a41b9bb9)
+++ b/src/bench/connectblock.cpp (date 17394512
...
(https://github.com/bitcoin/bitcoin/pull/31689#pullrequestreview-2614869602)
Concept ACK, please see my remaining comments:
<details>
<summary>Details</summary>
```patch
Index: src/bench/connectblock.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp
--- a/src/bench/connectblock.cpp (revision b3a64758bc09eb60858c23292ba7f1b9a41b9bb9)
+++ b/src/bench/connectblock.cpp (date 17394512
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954435541)
can be moved inside the benchmark to make sure previous run doesn't pollute the next one (the inner loop is unrolled and run multiple times so we should reduce dependencies)
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954435541)
can be moved inside the benchmark to make sure previous run doesn't pollute the next one (the inner loop is unrolled and run multiple times so we should reduce dependencies)
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954428945)
Seems unused now
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954428945)
Seems unused now