💬 LarryRuane commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476436648)
Yes, this would be very valuable. I'd like to attempt to get this going; @dergoegge, would that be okay? I made a related [comment](https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474874821) last week before I was aware of these websites (which are definitely better than what I suggested).
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476436648)
Yes, this would be very valuable. I'd like to attempt to get this going; @dergoegge, would that be okay? I made a related [comment](https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474874821) last week before I was aware of these websites (which are definitely better than what I suggested).
💬 pinheadmz commented on issue "Processing of new blocks slower than necessary due to overly selective peer selection":
(https://github.com/bitcoin/bitcoin/issues/21803#issuecomment-1476444242)
> slightly changing the code
Do you still have this patch available?
Is it the case that `> first cmpctblock received` is from a high-bandwidth CB peer but `> first node that announced the block (via headers).` is a low-bandwidth peer? And you're seeing bitcoin ignore the block message because it received the headers first and is requesting the block from the low-bandwidth peer?
(https://github.com/bitcoin/bitcoin/issues/21803#issuecomment-1476444242)
> slightly changing the code
Do you still have this patch available?
Is it the case that `> first cmpctblock received` is from a high-bandwidth CB peer but `> first node that announced the block (via headers).` is a low-bandwidth peer? And you're seeing bitcoin ignore the block message because it received the headers first and is requesting the block from the low-bandwidth peer?
💬 pinheadmz commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1476453733)
I don't understand how an "exportwatchonly" command is any more efficient or straight forward than `listdescriptors` -> `importdescriptors` ?
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1476453733)
I don't understand how an "exportwatchonly" command is any more efficient or straight forward than `listdescriptors` -> `importdescriptors` ?
💬 dergoegge commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476458871)
@LarryRuane cool, please do!
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476458871)
@LarryRuane cool, please do!
💬 dongcarl commented on issue ""Unable to parse settings.json" at startup after disk was full":
(https://github.com/bitcoin/bitcoin/issues/21340#issuecomment-1476464194)
One of my boxes running `v24.0.1` just encountered a low storage situation, and `/var/lib/bitcoind/settings.json` was truncated to 0 size.
From my `journalctl`:
```
Mar 20 11:33:05 hostname bitcoind[935629]: Error: Failed loading settings file:
Mar 20 11:33:05 hostname bitcoind[935629]: - Unable to parse settings file /var/lib/bitcoind/settings.json
Mar 20 11:33:05 hostname systemd[1]: bitcoind.service: Control process exited, code=exited, status=1/FAILURE
```
> This seems bad. I woul
...
(https://github.com/bitcoin/bitcoin/issues/21340#issuecomment-1476464194)
One of my boxes running `v24.0.1` just encountered a low storage situation, and `/var/lib/bitcoind/settings.json` was truncated to 0 size.
From my `journalctl`:
```
Mar 20 11:33:05 hostname bitcoind[935629]: Error: Failed loading settings file:
Mar 20 11:33:05 hostname bitcoind[935629]: - Unable to parse settings file /var/lib/bitcoind/settings.json
Mar 20 11:33:05 hostname systemd[1]: bitcoind.service: Control process exited, code=exited, status=1/FAILURE
```
> This seems bad. I woul
...
💬 pinheadmz commented on issue "Bitcoind repeatedly crashing at `UpdateTip` with no error message":
(https://github.com/bitcoin/bitcoin/issues/24483#issuecomment-1476467951)
@wbobeirne any updates on this issue or more clues to help debug?
(https://github.com/bitcoin/bitcoin/issues/24483#issuecomment-1476467951)
@wbobeirne any updates on this issue or more clues to help debug?
💬 instagibbs commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1476478682)
strong concept ACK on making it more clear on what flags are doing what(I find those names super unhelpful)
Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1476478682)
strong concept ACK on making it more clear on what flags are doing what(I find those names super unhelpful)
Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?
💬 pinheadmz commented on issue "Error: specified data directory "\\IP.Ad.re.ss\release\Folder"does not exist":
(https://github.com/bitcoin/bitcoin/issues/25868#issuecomment-1476482356)
I wonder if this is related to https://github.com/bitcoin/bitcoin/issues/27246 and fixed by https://github.com/bitcoin/bitcoin/pull/27273 ?
(https://github.com/bitcoin/bitcoin/issues/25868#issuecomment-1476482356)
I wonder if this is related to https://github.com/bitcoin/bitcoin/issues/27246 and fixed by https://github.com/bitcoin/bitcoin/pull/27273 ?
👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK 0d18142f595b43070bd07b608d43c9ed9b37cdcc. Suggested a few comment/whitespace tweaks, but this is also fine as-is.
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK 0d18142f595b43070bd07b608d43c9ed9b37cdcc. Suggested a few comment/whitespace tweaks, but this is also fine as-is.
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142325608)
In commit "wallet: Replace use of purpose strings with an enum" (ea3cc035ddf8f15fd18407231c3ff9edde8891c1)
s/address/addresses/
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142325608)
In commit "wallet: Replace use of purpose strings with an enum" (ea3cc035ddf8f15fd18407231c3ff9edde8891c1)
s/address/addresses/
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142308217)
In commit "wallet: Add wallet/types.h for simple public enum and struct types" (f00f5dad29d81b9700dd84ec08a6e72f915988a0)
Looks like there might be an extra 3 lines of space instead of 2 (but fine to keep if that's intentional)
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142308217)
In commit "wallet: Add wallet/types.h for simple public enum and struct types" (f00f5dad29d81b9700dd84ec08a6e72f915988a0)
Looks like there might be an extra 3 lines of space instead of 2 (but fine to keep if that's intentional)
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142331004)
In commit "doc: Release note for purpose string restriction" (0d18142f595b43070bd07b608d43c9ed9b37cdcc)
Maybe s/restricted/now restricted/ to be clear this is new behavior not background information.
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142331004)
In commit "doc: Release note for purpose string restriction" (0d18142f595b43070bd07b608d43c9ed9b37cdcc)
Maybe s/restricted/now restricted/ to be clear this is new behavior not background information.
💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1476485625)
> But approach wise I'd like us to preserve the intent of the user that the wallet is intended as `manual` (we should pick a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?
I agree, but I think that's orthogonal to this change and should be done in a followup.
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1476485625)
> But approach wise I'd like us to preserve the intent of the user that the wallet is intended as `manual` (we should pick a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?
I agree, but I think that's orthogonal to this change and should be done in a followup.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1142342689)
> I can always revert back to https://github.com/bitcoin/bitcoin/commit/f87c39399823e22c553b797cc66fa4063462a32b
Not sure. I will need to move those functions to blockman anyway for some other work, so dropping the changes will only kick the can down the road.
> Both are branching out a bit too much in my taste for the refactor we seek to achieve here.
I can take a look when time allows. In the meantime other reviewers can chime in.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1142342689)
> I can always revert back to https://github.com/bitcoin/bitcoin/commit/f87c39399823e22c553b797cc66fa4063462a32b
Not sure. I will need to move those functions to blockman anyway for some other work, so dropping the changes will only kick the can down the road.
> Both are branching out a bit too much in my taste for the refactor we seek to achieve here.
I can take a look when time allows. In the meantime other reviewers can chime in.
📝 hebasto opened a pull request: "build: Link `libbitcoinkernel` to `libbitcoin_util`"
(https://github.com/bitcoin/bitcoin/pull/27285)
This PR makes `libbitcoinkernel` reuse object files from `libbitcoin_util` instead of compiling them.
(https://github.com/bitcoin/bitcoin/pull/27285)
This PR makes `libbitcoinkernel` reuse object files from `libbitcoin_util` instead of compiling them.
💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476501591)
Git bisect points to 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 as the culprit (#19619). I verified that this commit triggers the error whereas its parent 3c815cfe54087fd139169161d2fd175e99840e6a doesn't.
It makes sense that this went unnoticed because `wallet_tests` was already suffering from a different memory leak at the time. cc @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476501591)
Git bisect points to 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 as the culprit (#19619). I verified that this commit triggers the error whereas its parent 3c815cfe54087fd139169161d2fd175e99840e6a doesn't.
It makes sense that this went unnoticed because `wallet_tests` was already suffering from a different memory leak at the time. cc @ryanofsky
💬 fanquake commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476501652)
> This PR makes libbitcoinkernel reuse object files from libbitcoin_util instead of compiling them.
Why
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476501652)
> This PR makes libbitcoinkernel reuse object files from libbitcoin_util instead of compiling them.
Why
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142351647)
fwiw: This is the "source", I think the repo you linked to is an outdated fork: https://sourceware.org/git/?p=systemtap.git;a=blob;f=includes/sys/sdt.h;h=4075a5ff1c3f09c3913615e62840595efe003c6d;hb=HEAD#l404
But yes, reading that comment I'd agree that it should work. I've added the following to try it but it doesn't compile. I think that's the reason why I added the `TRACEPOINT0()`.
```patch
diff --git a/src/test/util_trace_tests.cpp b/src/test/util_trace_tests.cpp
index 6e3e846b4..488d
...
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142351647)
fwiw: This is the "source", I think the repo you linked to is an outdated fork: https://sourceware.org/git/?p=systemtap.git;a=blob;f=includes/sys/sdt.h;h=4075a5ff1c3f09c3913615e62840595efe003c6d;hb=HEAD#l404
But yes, reading that comment I'd agree that it should work. I've added the following to try it but it doesn't compile. I think that's the reason why I added the `TRACEPOINT0()`.
```patch
diff --git a/src/test/util_trace_tests.cpp b/src/test/util_trace_tests.cpp
index 6e3e846b4..488d
...
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142353299)
Plan to revisit this once I have the time to experiment
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142353299)
Plan to revisit this once I have the time to experiment
💬 0xB10C commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1476519399)
ACK 4b7aec2951fe4595946cdc804b0dec1921d79d05
Reviewed changes since my last ACK with `git range-diff d0c6da92c...4b7aec2951`:
- This adds the entry time to the `mempool:removed` transactions. The docs and the tests are updated accordingly.
- Code reviewed that the `mempool_monitor.py` demo now doesn't store events forever and let the script run for >10min.
- Ran the `interface_usdt_mempool.py` test
The failing CI job looks unrelated.
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1476519399)
ACK 4b7aec2951fe4595946cdc804b0dec1921d79d05
Reviewed changes since my last ACK with `git range-diff d0c6da92c...4b7aec2951`:
- This adds the entry time to the `mempool:removed` transactions. The docs and the tests are updated accordingly.
- Code reviewed that the `mempool_monitor.py` demo now doesn't store events forever and let the script run for >10min.
- Ran the `interface_usdt_mempool.py` test
The failing CI job looks unrelated.