💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720088411)
Changed to use that asmap, although did not regenerate the seeds.
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720088411)
Changed to use that asmap, although did not regenerate the seeds.
📝 mzumsande opened a pull request: "validation: improve m_best_header tracking"
(https://github.com/bitcoin/bitcoin/pull/30666)
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seem up-to-date), so it be
...
(https://github.com/bitcoin/bitcoin/pull/30666)
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seem up-to-date), so it be
...
💬 mzumsande commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2293902754)
I'll close this for now, large parts of this PR are contained in #30666 which actually fixes m_best_header tracking instead of just documenting its limitations.
The last two commits of this PR are not included in #30666 and fix a separate issue - I might open a separate PR for this at a later time.
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2293902754)
I'll close this for now, large parts of this PR are contained in #30666 which actually fixes m_best_header tracking instead of just documenting its limitations.
The last two commits of this PR are not included in #30666 and fix a separate issue - I might open a separate PR for this at a later time.
✅ mzumsande closed a pull request: "validation: Improve, document and test logic for chains building on invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/30207)
(https://github.com/bitcoin/bitcoin/pull/30207)
💬 sipa commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1720132804)
Is `m_best_header` necessarily a descendant of `pindexNew` here? If not, this loop will walk past genesis.
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1720132804)
Is `m_best_header` necessarily a descendant of `pindexNew` here? If not, this loop will walk past genesis.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2294005237)
(Reposting at PR level instead of review level)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2294005237)
(Reposting at PR level instead of review level)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903
...
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1720227562)
Ok, mystery solved. These adhoc initialization costs do not interact with the `sqrt(2^k)+1` rule, because that rule is at the per-candidate-set level, while the adhoc costs are applied at the linearization level. It does in theory interact with the derived $\sqrt{12 * 2^n}+n$ rule, but for $n \leq 3$ the linearization code is optimal without any search, and for $n \geq 4$, the formula is sufficiently overshooting to accomodate the ad-hoc costs. Still, I've changed the test to use precomputed lim
...
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1720227562)
Ok, mystery solved. These adhoc initialization costs do not interact with the `sqrt(2^k)+1` rule, because that rule is at the per-candidate-set level, while the adhoc costs are applied at the linearization level. It does in theory interact with the derived $\sqrt{12 * 2^n}+n$ rule, but for $n \leq 3$ the linearization code is optimal without any search, and for $n \geq 4$, the formula is sufficiently overshooting to accomodate the ad-hoc costs. Still, I've changed the test to use precomputed lim
...
💬 hebasto commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2294101230)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/334.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2294101230)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/334.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720284745)
Looking at the actual code generated by GCC 14.2 (https://godbolt.org/z/8oTfPKE4q), I get the following diff from adding `[[likely]]`:
```diff
@@ -1,6 +1,13 @@
mov rax, rdi
shr rax, 33
- je .L8
+ jne .L2
+ movsx rax, edx
+ mov esi, esi
+ xor edx, edx
+ imul rax, rdi
+ div rsi
+ ret
+.L2:
movsx r10, edx
sub rsp, 24
mov rax, r10
@@ -24,
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720284745)
Looking at the actual code generated by GCC 14.2 (https://godbolt.org/z/8oTfPKE4q), I get the following diff from adding `[[likely]]`:
```diff
@@ -1,6 +1,13 @@
mov rax, rdi
shr rax, 33
- je .L8
+ jne .L2
+ movsx rax, edx
+ mov esi, esi
+ xor edx, edx
+ imul rax, rdi
+ div rsi
+ ret
+.L2:
movsx r10, edx
sub rsp, 24
mov rax, r10
@@ -24,
...
💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720294520)
I personally view it as documentation and would definitely keep it here
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720294520)
I personally view it as documentation and would definitely keep it here
💬 mzumsande commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2294229191)
I opened #30666
> @mzumsande, guess that your upcoming PR will continue containing the same shared commit "validation: mark blocks between m_best_header and invalid block as BLOCK_FAILED_CHILD" ?
Yes, I included that, without these block marked as BLOCK_FAILED_CHILD it `m_best_header` could still be incorrect.
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2294229191)
I opened #30666
> @mzumsande, guess that your upcoming PR will continue containing the same shared commit "validation: mark blocks between m_best_header and invalid block as BLOCK_FAILED_CHILD" ?
Yes, I included that, without these block marked as BLOCK_FAILED_CHILD it `m_best_header` could still be incorrect.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720339477)
Done!
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1720339477)
Done!
📝 allanperlee opened a pull request: "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`"
(https://github.com/bitcoin/bitcoin/pull/30667)
Addressing issue #30600, this pull request simplifies the `send_to_address` functional test in rpc_signrawtransactionwithkey.py. The code is more terse with a MiniWallet object, its ADDRESS_OP_TRUE mode and `send_to` method on line 54, thus removing the "low-level" details of the transaction. Suggestions will be addressed. Thank you for reviewing!
(https://github.com/bitcoin/bitcoin/pull/30667)
Addressing issue #30600, this pull request simplifies the `send_to_address` functional test in rpc_signrawtransactionwithkey.py. The code is more terse with a MiniWallet object, its ADDRESS_OP_TRUE mode and `send_to` method on line 54, thus removing the "low-level" details of the transaction. Suggestions will be addressed. Thank you for reviewing!
✅ ryanofsky closed a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364)
(https://github.com/bitcoin/bitcoin/pull/30364)
💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2294347140)
I lost enthusiasm for this PR, so will close it, but tagging as up for grabs, and will happily review if someone else wants to pick it up and respond to the earlier comments, particularly the individual logging improvements suggested https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2182052463.
---
The thing I like about this PR is that it disentangles log message ___priority levels___ from log message ___conditions___.
As I see it, **priority levels** can distinguish bet
...
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2294347140)
I lost enthusiasm for this PR, so will close it, but tagging as up for grabs, and will happily review if someone else wants to pick it up and respond to the earlier comments, particularly the individual logging improvements suggested https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2182052463.
---
The thing I like about this PR is that it disentangles log message ___priority levels___ from log message ___conditions___.
As I see it, **priority levels** can distinguish bet
...
💬 ryanofsky commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2294365452)
re: https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2231748835
I think I might not have enough information to understand the weak nack, because I don't actually see a reason in there to reject a documentation fix that is only dropping advice to interpret `Error` as "fatal" and `Warning` as "nonfatal".
The advice is not followed by current code, but more importantly, I don't think anybody (even the original author) currently thinks this advice is something we should try to implem
...
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2294365452)
re: https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2231748835
I think I might not have enough information to understand the weak nack, because I don't actually see a reason in there to reject a documentation fix that is only dropping advice to interpret `Error` as "fatal" and `Warning` as "nonfatal".
The advice is not followed by current code, but more importantly, I don't think anybody (even the original author) currently thinks this advice is something we should try to implem
...
👍 ryanofsky approved a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2243634116)
Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2243634116)
Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1720427422)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563783
> q in [f1c1b37](https://github.com/bitcoin/bitcoin/commit/f1c1b37d2247eed49156c0467daa68aa38497bb8): Can you explain why this change is correct?
Of course it is not correct. I only noticed that this function was updating `g_insecure_rand_ctx` and didn't notice it was updating global rng state. Thanks for fixing!
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1720427422)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563783
> q in [f1c1b37](https://github.com/bitcoin/bitcoin/commit/f1c1b37d2247eed49156c0467daa68aa38497bb8): Can you explain why this change is correct?
Of course it is not correct. I only noticed that this function was updating `g_insecure_rand_ctx` and didn't notice it was updating global rng state. Thanks for fixing!
👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2243660929)
Code review ACK 164732369fc5ecf4c7705136a2de884419023b5a. New commit replacing SeedRandomForTest is very nice and clarifies the code.
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2243660929)
Code review ACK 164732369fc5ecf4c7705136a2de884419023b5a. New commit replacing SeedRandomForTest is very nice and clarifies the code.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720368838)
Was removed it in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998 but then I went one step further and deferred the assert to the `base_blob` `Span`-ctor, making the comment more relevant again. Brought back now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720368838)
Was removed it in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998 but then I went one step further and deferred the assert to the `base_blob` `Span`-ctor, making the comment more relevant again. Brought back now.