💬 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
💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019521714)
Yeah, guess that pushes it too far into the "not worth it"
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019521714)
Yeah, guess that pushes it too far into the "not worth it"
💬 hodlinator commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2762783273)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
Maybe one could add yet another option to make manually specified nodes be treated with extra leniency?
First I was thinking that maybe we could treat `-connect`-nodes vs `-addnodes` differently automatically in this respect. But it would be more natural for `-connect` to be protected, and your (jonatack's) use-case is `-addnode` fro
...
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2762783273)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
Maybe one could add yet another option to make manually specified nodes be treated with extra leniency?
First I was thinking that maybe we could treat `-connect`-nodes vs `-addnodes` differently automatically in this respect. But it would be more natural for `-connect` to be protected, and your (jonatack's) use-case is `-addnode` fro
...
💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019533582)
Hmm. I didn't understand how BlockBuilder was implemented when I wrote that; BlockBuilder maintains an ordered tree of every main chunk, so is O(num_chunks) in size/performance, which is fine. What I had in mind conceptually about cluster mempool is that you'd have a structure that was O(num_clusters) that you could query for the the best/worst chunk. That would be O(n log(n)) setup, then O(log(n)) for the equivalent of a "Next()" call, for n=num_clusters. But that's not superior to how BlockBui
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019533582)
Hmm. I didn't understand how BlockBuilder was implemented when I wrote that; BlockBuilder maintains an ordered tree of every main chunk, so is O(num_chunks) in size/performance, which is fine. What I had in mind conceptually about cluster mempool is that you'd have a structure that was O(num_clusters) that you could query for the the best/worst chunk. That would be O(n log(n)) setup, then O(log(n)) for the equivalent of a "Next()" call, for n=num_clusters. But that's not superior to how BlockBui
...
💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019536181)
> It's a possibility, but you mean this in addition to `BlockBuildImpl` itself also incrementing the observers count, right? Because it holds an iterator to the chunk index in GraphImpl too.
Yes. (though, technically, if `BlockBuildImpl` stores its own RefsVector it could avoid explicitly incrementing the observers count, because RefsVector does that for it)
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019536181)
> It's a possibility, but you mean this in addition to `BlockBuildImpl` itself also incrementing the observers count, right? Because it holds an iterator to the chunk index in GraphImpl too.
Yes. (though, technically, if `BlockBuildImpl` stores its own RefsVector it could avoid explicitly incrementing the observers count, because RefsVector does that for it)
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2019537391)
Do you mean the `lld` instruction? I find that confusing to keep that only there. I would prefer if you could keep the instruction here as well and maybe adapt the `lld` part to only talk about installing `lld` potentially also making clear that using brew is just an example, just like here.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2019537391)
Do you mean the `lld` instruction? I find that confusing to keep that only there. I would prefer if you could keep the instruction here as well and maybe adapt the `lld` part to only talk about installing `lld` potentially also making clear that using brew is just an example, just like here.
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2019538758)
Ok, fine to fix the typo but you should keep the structure. That was aiming to maintain the 80 character limit which is used throughout most of the text in this file and I think many people prefer using that limit.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2019538758)
Ok, fine to fix the typo but you should keep the structure. That was aiming to maintain the 80 character limit which is used throughout most of the text in this file and I think many people prefer using that limit.
✅ fanquake closed a pull request: "Fix legacy migration bug"
(https://github.com/bitcoin/bitcoin/pull/32161)
(https://github.com/bitcoin/bitcoin/pull/32161)
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2762968613)
Seeing this output (for all translations) while Guix building. Is it an issue:
```bash
[ 52%] Automatic MOC and UIC for target bitcoinqt
AutoUic: Detected locale "C" with character encoding "ANSI_X3.4-1968", which is not UTF-8.
Qt depends on a UTF-8 locale, but has failed to switch to one.
If this causes problems, reconfigure your locale. See the locale(1) manual
for more information.
<snip>
[ 52%] Generating locale/bitcoin_fi.qm
Detected locale "C" with character encoding "ANSI_X3.4-19
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2762968613)
Seeing this output (for all translations) while Guix building. Is it an issue:
```bash
[ 52%] Automatic MOC and UIC for target bitcoinqt
AutoUic: Detected locale "C" with character encoding "ANSI_X3.4-1968", which is not UTF-8.
Qt depends on a UTF-8 locale, but has failed to switch to one.
If this causes problems, reconfigure your locale. See the locale(1) manual
for more information.
<snip>
[ 52%] Generating locale/bitcoin_fi.qm
Detected locale "C" with character encoding "ANSI_X3.4-19
...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019656793)
Yeah, I think I considered an on-the-fly heap at some point, and it's probable that that actually has overall lower runtime costs than this (due do better memory locality and less allocation overhead than a tree-structured set), but being to do the bulk of the work ahead of time is nice.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019656793)
Yeah, I think I considered an on-the-fly heap at some point, and it's probable that that actually has overall lower runtime costs than this (due do better memory locality and less allocation overhead than a tree-structured set), but being to do the bulk of the work ahead of time is nice.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2763012132)
Guix Build (x86_64):
```bash
6ea4a76be3383337e57d6a12450bd589776ebb3fd0d9161347766ef845241e13 guix-build-c4861570e468/output/aarch64-linux-gnu/SHA256SUMS.part
3eb7656483dfe47fa6b7cf40bceb3decda73474c813edb224d42840adb8b49d6 guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu-debug.tar.gz
0aa522010efd138d78eeac0a8ea15df469298c50afaa2451dece78c564546cac guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu.tar.gz
5e1e833
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2763012132)
Guix Build (x86_64):
```bash
6ea4a76be3383337e57d6a12450bd589776ebb3fd0d9161347766ef845241e13 guix-build-c4861570e468/output/aarch64-linux-gnu/SHA256SUMS.part
3eb7656483dfe47fa6b7cf40bceb3decda73474c813edb224d42840adb8b49d6 guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu-debug.tar.gz
0aa522010efd138d78eeac0a8ea15df469298c50afaa2451dece78c564546cac guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu.tar.gz
5e1e833
...
💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019711805)
Thinking some more about this... I guess the advantage of the on-the-fly heap would be that you may never have to calculate the ordering for some chunks in the middle of the mempool (if they get rbf'ed, or conflicted by a block tx, or if they expire, or if they get cpfp'ed and thus brought to the front of the mempool, rather than just waiting until the best block feerate drops to their level, or you stop and restart your bitcoind before they get mined). Probably few enough txs hit those special
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019711805)
Thinking some more about this... I guess the advantage of the on-the-fly heap would be that you may never have to calculate the ordering for some chunks in the middle of the mempool (if they get rbf'ed, or conflicted by a block tx, or if they expire, or if they get cpfp'ed and thus brought to the front of the mempool, rather than just waiting until the best block feerate drops to their level, or you stop and restart your bitcoind before they get mined). Probably few enough txs hit those special
...
📝 fanquake locked a pull request: "5e8bc97cdc6dd8d7be10f0ac8c1b46d2c2fd1547"
(https://github.com/bitcoin/bitcoin/pull/32127)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32127)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...