💬 l0rinc commented on pull request "[IBD] flush UTXOs in bigger batches based on dbcache size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2051557904)
Updated it and rebased to fix build - please re-review.
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2051557904)
Updated it and rebased to fix build - please re-review.
💬 l0rinc commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2816824275)
Concept ACK, depends on https://github.com/bitcoin/bitcoin/pull/32309 to make the build pass
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2816824275)
Concept ACK, depends on https://github.com/bitcoin/bitcoin/pull/32309 to make the build pass
💬 venpisey commented on pull request "Make TxGraph fuzz tests more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32191#issuecomment-2816825465)
5167170015919920
(https://github.com/bitcoin/bitcoin/pull/32191#issuecomment-2816825465)
5167170015919920
💬 jonatack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2816826222)
Latest changes since my last review LGTM. I plan to review the code more closely.
Empirically, running a node on this branch, it looks like over time most peers settle into less than 0.5 per milles, a few are higher, with maybe one peer around 4. These higher-load connections seem to more often be outbound ones.
(Edit, updated the branch with the -netinfo commit with a few improvements and to also only display cpu load for a peer (rounded to nearest integer) if it is non-zero (e.g. >= 0.5
...
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2816826222)
Latest changes since my last review LGTM. I plan to review the code more closely.
Empirically, running a node on this branch, it looks like over time most peers settle into less than 0.5 per milles, a few are higher, with maybe one peer around 4. These higher-load connections seem to more often be outbound ones.
(Edit, updated the branch with the -netinfo commit with a few improvements and to also only display cpu load for a peer (rounded to nearest integer) if it is non-zero (e.g. >= 0.5
...
💬 l0rinc commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2051562712)
I could, of course, for now I was going for simplifying the diff, not make the code more readable. I can of course do that later, but for now I'm looking for the simplest diff which demonstrates the problem and the proposed solution, and after I'm sure there's general support for it, we can sneak in code refactors as well.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2051562712)
I could, of course, for now I was going for simplifying the diff, not make the code more readable. I can of course do that later, but for now I'm looking for the simplest diff which demonstrates the problem and the proposed solution, and after I'm sure there's general support for it, we can sneak in code refactors as well.
💬 sipa commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816837610)
Nit: in title/description of the PR, I don't think it's accurate to say "on the stack". While there are occasionally CScript objects created on the stack, the vast majority is inside CCoinsCacheDb, where they are still heap-allocated. The benefit of the change here is that some of these would no longer need a *separate* heap allocation per CScript.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816837610)
Nit: in title/description of the PR, I don't think it's accurate to say "on the stack". While there are occasionally CScript objects created on the stack, the vast majority is inside CCoinsCacheDb, where they are still heap-allocated. The benefit of the change here is that some of these would no longer need a *separate* heap allocation per CScript.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816840182)
Thank you, I have used inline/stack interchangeably - this is a lot better, appreciate the correction.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2816840182)
Thank you, I have used inline/stack interchangeably - this is a lot better, appreciate the correction.
💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051568605)
Pushed again, please recheck
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051568605)
Pushed again, please recheck
🤔 mzumsande reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2780087926)
Would be good to explain 1.) Why it failed before and 2.) How this is fixed
Looking at the commits, it seems that there are two things going on that seem contradictory:
Making sure that the benchmarks could have multiple invocations (commit 1 and 3), and making sure that it actually only runs once (commit 2).
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2780087926)
Would be good to explain 1.) Why it failed before and 2.) How this is fixed
Looking at the commits, it seems that there are two things going on that seem contradictory:
Making sure that the benchmarks could have multiple invocations (commit 1 and 3), and making sure that it actually only runs once (commit 2).
💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816868935)
Thanks, I have updated the description, let me know if it answered your questions.
Short answer anyway: we have to clean up regardless of the execution count - and the benchmark seems to have been designed for a single execution anyway, otherwise recreating the wallet might have been too costly.
Let me know if you have a better idea.
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816868935)
Thanks, I have updated the description, let me know if it answered your questions.
Short answer anyway: we have to clean up regardless of the execution count - and the benchmark seems to have been designed for a single execution anyway, otherwise recreating the wallet might have been too costly.
Let me know if you have a better idea.
💬 fjahr commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816873696)
> I’ll investigate this further and determine the best way to implement a fix.
@EniRox001 I think we spoke on IRC but you went offline and I couldn't send you this last message. I took a brief look at the failure and it seems to me that the problem is that node 2 is being used in the reorg test previously. That's why it's on a different chain than the other nodes and can't fetch the last pruned block from the other nodes because these other nodes don't know this block. Fixing the typo surfaces
...
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816873696)
> I’ll investigate this further and determine the best way to implement a fix.
@EniRox001 I think we spoke on IRC but you went offline and I couldn't send you this last message. I took a brief look at the failure and it seems to me that the problem is that node 2 is being used in the reorg test previously. That's why it's on a different chain than the other nodes and can't fetch the last pruned block from the other nodes because these other nodes don't know this block. Fixing the typo surfaces
...
💬 fjahr commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816874110)
@EniRox001 If you want to you could also introduce a new node that is not involved in any other test, some might prefer this as cleaner since it avoids such issues. But that's up to you.
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816874110)
@EniRox001 If you want to you could also introduce a new node that is not involved in any other test, some might prefer this as cleaner since it avoids such issues. But that's up to you.
💬 GYGY072H commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816895006)
Look honestly I thought this was A computer class program I have no fkg
Idea wat is going on I don't even know the names of the bottom's this is
ridiculous I thought I need to take a class so am not embarrassed of not
knowing how to use a computer int life all I use is my phone even that is
difficult for me can you can you explain to me what is this? Am kinda
scared am not and American and this looks funny
On Sat, Apr 19, 2025, 5:37 PM Fabian Jahr ***@***.***> wrote:
> @EniRox001 <h
...
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2816895006)
Look honestly I thought this was A computer class program I have no fkg
Idea wat is going on I don't even know the names of the bottom's this is
ridiculous I thought I need to take a class so am not embarrassed of not
knowing how to use a computer int life all I use is my phone even that is
difficult for me can you can you explain to me what is this? Am kinda
scared am not and American and this looks funny
On Sat, Apr 19, 2025, 5:37 PM Fabian Jahr ***@***.***> wrote:
> @EniRox001 <h
...
👍 theStack approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2780108332)
re-ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
case-sensitivity yocto-nit: in commit subject for 6268bde0af0aa9cfb3df08d6b9b67fdffa2a1a12: s/BIP32PUbKeyProvider/BIP32PubkeyProvider/
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2780108332)
re-ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
case-sensitivity yocto-nit: in commit subject for 6268bde0af0aa9cfb3df08d6b9b67fdffa2a1a12: s/BIP32PUbKeyProvider/BIP32PubkeyProvider/
💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051609620)
ah yes messed up the rebase..fixed now.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051609620)
ah yes messed up the rebase..fixed now.
💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051609798)
good idea, changed it.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051609798)
good idea, changed it.
💬 mzumsande commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816951357)
Thanks, I didn't realize that there were multiple independent issues - makes sense now, Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816951357)
Thanks, I didn't realize that there were multiple independent issues - makes sense now, Concept ACK.
⚠️ vicortar opened an issue: "Critical Discrepancy: Bitcoin Core Node Sees UTXO via scantxoutset, BUT listunspent & Sparrow Wallet Fail; Address Undrivable from LND Key?"
(https://github.com/bitcoin/bitcoin/issues/32311)
Hello LND Developers and Community,
I'm facing a highly unusual and critical issue where on-chain funds associated with a past LND channel closure are inaccessible, despite my Bitcoin Core node (v29.0.0, RPi5, fully synced after fresh IBD, txindex=1) confirming the UTXO's existence via scantxoutset. However, both listunspent and external wallet software (Sparrow) fail to see this UTXO, and crucially, the address holding the funds cannot be derived from my LND root key using standard methods.
Sys
...
(https://github.com/bitcoin/bitcoin/issues/32311)
Hello LND Developers and Community,
I'm facing a highly unusual and critical issue where on-chain funds associated with a past LND channel closure are inaccessible, despite my Bitcoin Core node (v29.0.0, RPi5, fully synced after fresh IBD, txindex=1) confirming the UTXO's existence via scantxoutset. However, both listunspent and external wallet software (Sparrow) fail to see this UTXO, and crucially, the address holding the funds cannot be derived from my LND root key using standard methods.
Sys
...
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2817037407)
Finished a full IBD with default dbcache=450 on HDD
<details>
<summary>Benchmark</summary>
```bash
COMMITS="156f9913a26598009c8356c34548002e6a6aba02 a2a3e0cfd8549e3be03fd81e75ebb7527e9345d7"; \
STOP_HEIGHT=888888; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(for c in $COMMITS; do git fetch origin $c -q && git log -1 --pretty=format:'%h %s' $c || exit 1; done) && \
hyperfine \
--sort 'command' \
--run
...
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2817037407)
Finished a full IBD with default dbcache=450 on HDD
<details>
<summary>Benchmark</summary>
```bash
COMMITS="156f9913a26598009c8356c34548002e6a6aba02 a2a3e0cfd8549e3be03fd81e75ebb7527e9345d7"; \
STOP_HEIGHT=888888; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(for c in $COMMITS; do git fetch origin $c -q && git log -1 --pretty=format:'%h %s' $c || exit 1; done) && \
hyperfine \
--sort 'command' \
--run
...
🤔 i-am-yuvi reviewed a pull request: "build: Fix `macdeployqtplus` after switching to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/32287#pullrequestreview-2780171994)
tACK b2f1c20d24ee6cae65e7bbff60091b39cc2694b4
```
...
...
+ Deploying plugins +
Processing plugin platforms/libqminimal.dylib ...
/Library/Developer/CommandLineTools/usr/bin/strip: warning: changes being made to the file will invalidate the code signature in: /Users/yuvrajchhetri/Desktop/bitcoin/build/dist/Bitcoin-Qt.app/Contents/PlugIns/platforms/libqminimal.dylib
/Library/Developer/CommandLineTools/usr/bin/install_name_tool: warning: changes being made to the file will invalidate the
...
(https://github.com/bitcoin/bitcoin/pull/32287#pullrequestreview-2780171994)
tACK b2f1c20d24ee6cae65e7bbff60091b39cc2694b4
```
...
...
+ Deploying plugins +
Processing plugin platforms/libqminimal.dylib ...
/Library/Developer/CommandLineTools/usr/bin/strip: warning: changes being made to the file will invalidate the code signature in: /Users/yuvrajchhetri/Desktop/bitcoin/build/dist/Bitcoin-Qt.app/Contents/PlugIns/platforms/libqminimal.dylib
/Library/Developer/CommandLineTools/usr/bin/install_name_tool: warning: changes being made to the file will invalidate the
...