💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375949)
Done.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375949)
Done.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375612)
This has been rewritten.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375612)
This has been rewritten.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019377271)
Done.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019377271)
Done.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019379608)
That's strange, because the builder outliving the graph is definitely a problem (the observer counter in the graph, which has been destroyed, will be subtracted from). I don't see an easy way of allowing this (it'd need the graph to maintain a set of observers, which seems overkill).
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019379608)
That's strange, because the builder outliving the graph is definitely a problem (the observer counter in the graph, which has been destroyed, will be subtracted from). I don't see an easy way of allowing this (it'd need the graph to maintain a set of observers, which seems overkill).
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019379802)
Done.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019379802)
Done.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019380171)
Done.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019380171)
Done.
💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2019431503)
We're in Python here though. I spent a couple of hours with an LLM in mid-February to brute force this embryo:
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2025/02/assert_ergonomics
<details><summary>Output examples</summary>
Current `assert_equal()`-behavior:
```
2025-03-28T21:15:36.379000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, i
...
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2019431503)
We're in Python here though. I spent a couple of hours with an LLM in mid-February to brute force this embryo:
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2025/02/assert_ergonomics
<details><summary>Output examples</summary>
Current `assert_equal()`-behavior:
```
2025-03-28T21:15:36.379000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, i
...
💬 davidgumberg commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019445025)
Was this generated with an LLM?
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019445025)
Was this generated with an LLM?
💬 davidgumberg commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019448985)
Could you please explain why you made these changes?
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019448985)
Could you please explain why you made these changes?
💬 arejula27 commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2019460884)
I was discussing with @hodlinator and @Prabhat1308 that a Coinbase UTXO cannot be spent until 100 blocks have been added after it. Wouldn't it be better to set this to static_cast<uint32_t>(nHeight +99 );? This could simplify the logic for the Coinbase spend condition and potentially make mempool and transaction validation more efficient
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2019460884)
I was discussing with @hodlinator and @Prabhat1308 that a Coinbase UTXO cannot be spent until 100 blocks have been added after it. Wouldn't it be better to set this to static_cast<uint32_t>(nHeight +99 );? This could simplify the logic for the Coinbase spend condition and potentially make mempool and transaction validation more efficient
💬 davidgumberg commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019462360)
I'm not sure that this change preserves the intent of the error, could you explain why you came to the conclusion that this situation is not an internal bug in our handling of the wallet file?
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019462360)
I'm not sure that this change preserves the intent of the error, could you explain why you came to the conclusion that this situation is not an internal bug in our handling of the wallet file?
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2019470411)
Done here: https://github.com/bitcoin/bitcoin/pull/32150/commits/e2a4d86131e8f1661eb450c50b8072147965b111
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2019470411)
Done here: https://github.com/bitcoin/bitcoin/pull/32150/commits/e2a4d86131e8f1661eb450c50b8072147965b111
💬 davidgumberg commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#issuecomment-2762678753)
Hi @olaristo109!
Thanks for taking a look at this, you mention adding "a unit test to 'wallet_tests.cpp' to ensure the fix is robust and to prevent future regressions.", but I don't see this in the PR, and the PR contains some unnecessary/unrelated changes, like fixing the "typo" (which is not a typo) in the release notes document.
I am also not convinced that this actually solves the underlying issue in #32112, I left a comment above, but I'm not convinced that the existing [`Assume`, com
...
(https://github.com/bitcoin/bitcoin/pull/32161#issuecomment-2762678753)
Hi @olaristo109!
Thanks for taking a look at this, you mention adding "a unit test to 'wallet_tests.cpp' to ensure the fix is robust and to prevent future regressions.", but I don't see this in the PR, and the PR contains some unnecessary/unrelated changes, like fixing the "typo" (which is not a typo) in the release notes document.
I am also not convinced that this actually solves the underlying issue in #32112, I left a comment above, but I'm not convinced that the existing [`Assume`, com
...
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2019486082)
You didn't change the behavior. The `db_batch` object is alive **while** `Flush()` and `DeleteRecords()` are called. My suggestion was to scope each single `HasLegacyRecords()` call so it does not interfere with them.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2019486082)
You didn't change the behavior. The `db_batch` object is alive **while** `Flush()` and `DeleteRecords()` are called. My suggestion was to scope each single `HasLegacyRecords()` call so it does not interfere with them.
💬 furszy commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#issuecomment-2762708825)
The code makes no sense. Time is a scarce resource. Would suggest to just close the PR.
(https://github.com/bitcoin/bitcoin/pull/32161#issuecomment-2762708825)
The code makes no sense. Time is a scarce resource. Would suggest to just close the PR.
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458266)
I'm making this and two similar changes above (for `std::unordered_set` and `std::unordered_map` because CI debug prints show that on some platforms (macOS and Win64), each element of the bucket array uses 16 bytes (likely two pointers) rather than 8 bytes (one pointer). This is probably because the bucket lists are doubly-linked. (This gives a performance advantage for removal; it's not necessary to hash the key to find the bucket, then walk that bucket's list looking for the element being remo
...
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458266)
I'm making this and two similar changes above (for `std::unordered_set` and `std::unordered_map` because CI debug prints show that on some platforms (macOS and Win64), each element of the bucket array uses 16 bytes (likely two pointers) rather than 8 bytes (one pointer). This is probably because the bucket lists are doubly-linked. (This gives a performance advantage for removal; it's not necessary to hash the key to find the bucket, then walk that bucket's list looking for the element being remo
...
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019446723)
I had to change this back to `~(step - 1)` because CI complained; some compilers don't accept the negation of an unsigned type.
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019446723)
I had to change this back to `~(step - 1)` because CI complained; some compilers don't accept the negation of an unsigned type.
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458527)
see review comment below
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458527)
see review comment below
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019499591)
I just realized that I can't add this test commit after the functional commit, because then the functional commit doesn't pass on some platforms (macOS is one for sure). So it would have to be part of the functional commit. Does that sound okay to you?
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019499591)
I just realized that I can't add this test commit after the functional commit, because then the functional commit doesn't pass on some platforms (macOS is one for sure). So it would have to be part of the functional commit. Does that sound okay to you?
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458369)
see review comment below
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2019458369)
see review comment below