💬 laanwj commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517278954)
As long as these are linked dynamically, this will increase the minimum versions that need to be installed on the target system. i'm not sure how much of a problem this is, if we have a reason to need newer functions of these interfaces.
(at the least this will have to be tested on common target distro versions)
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517278954)
As long as these are linked dynamically, this will increase the minimum versions that need to be installed on the target system. i'm not sure how much of a problem this is, if we have a reason to need newer functions of these interfaces.
(at the least this will have to be tested on common target distro versions)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517285109)
CI failed because it runs against master and #33575 has been merged, which added `interruptWait` at position `@11`. I rebased and bumped `getCoinbase()` to `@12`.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517285109)
CI failed because it runs against master and #33575 has been merged, which added `interruptWait` at position `@11`. I rebased and bumped `getCoinbase()` to `@12`.
💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517290596)
@laanwj my intention is to have these linked statically (within the v31 cycle), this has just been pulled out of #33537. If you'd prefer to review any changes as a single unit in #33537, I'm happy to keep everything together there.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517290596)
@laanwj my intention is to have these linked statically (within the v31 cycle), this has just been pulled out of #33537. If you'd prefer to review any changes as a single unit in #33537, I'm happy to keep everything together there.
💬 laanwj commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517330032)
Right! It's a good idea to pull it out. My point is just that we probably only want to merge this after #33537.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517330032)
Right! It's a good idea to pull it out. My point is just that we probably only want to merge this after #33537.
💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514589535)
In other words, each newly requested temple is _copied_ from the cache? Sounds good.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514589535)
In other words, each newly requested temple is _copied_ from the cache? Sounds good.
💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517375440)
> My point is just that we probably only want to merge this after https://github.com/bitcoin/bitcoin/pull/33537.
I'll invert the PR stack.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517375440)
> My point is just that we probably only want to merge this after https://github.com/bitcoin/bitcoin/pull/33537.
I'll invert the PR stack.
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514611258)
If you want to modify the template, yes. But for things like sendtemplate, fee estimation, that just want to retrieve data from the template they don't need a copy.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514611258)
If you want to modify the template, yes. But for things like sendtemplate, fee estimation, that just want to retrieve data from the template they don't need a copy.
📝 fanquake converted_to_draft a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)
Based on #33537.
(https://github.com/bitcoin/bitcoin/pull/33851)
Based on #33537.
📝 stickies-v opened a pull request: "kernel: add btck_block_tree_entry_equals"
(https://github.com/bitcoin/bitcoin/pull/33855)
`BlockTreeEntry` objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.
Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? `BlockTreeEntry` is the main one I've needed this for
...
(https://github.com/bitcoin/bitcoin/pull/33855)
`BlockTreeEntry` objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.
Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? `BlockTreeEntry` is the main one I've needed this for
...
👋 waketraindev's pull request is ready for review: "Prevent re-execution of sensitive commands from console history"
(https://github.com/bitcoin-core/gui/pull/909)
(https://github.com/bitcoin-core/gui/pull/909)
💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517459399)
re: https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033
Thanks @TheCharlatan. I think the approach you took in #30595 of making the kernel API mirror the current `logging.h` API was the best one to take as a starting point. #30595 covers a large surface area, and it would have been asking too much of reviewers to evaluate all the different ways the kernel API and existing APIs could or should diverge in one PR. It should be more manageable to make kernel API improvements in
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517459399)
re: https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033
Thanks @TheCharlatan. I think the approach you took in #30595 of making the kernel API mirror the current `logging.h` API was the best one to take as a starting point. #30595 covers a large surface area, and it would have been asking too much of reviewers to evaluate all the different ways the kernel API and existing APIs could or should diverge in one PR. It should be more manageable to make kernel API improvements in
...
💬 fanquake commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517462890)
Note that this will still fail:
```bash
[ 74%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp: In member function 'void btck_transaction_tests::test_method()':
/bitcoin/src/test/kernel/test_kernel.cpp:448:34: error: no match for 'operator|' (operand types are 'btck::Range<btck::Transaction, &btck::TransactionApi<btck::Transaction>::CountOutputs, &btck::TransactionApi<btck::Transaction>::GetOutput>' and 'std::ranges::views
...
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517462890)
Note that this will still fail:
```bash
[ 74%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp: In member function 'void btck_transaction_tests::test_method()':
/bitcoin/src/test/kernel/test_kernel.cpp:448:34: error: no match for 'operator|' (operand types are 'btck::Range<btck::Transaction, &btck::TransactionApi<btck::Transaction>::CountOutputs, &btck::TransactionApi<btck::Transaction>::GetOutput>' and 'std::ranges::views
...
📝 Eunovo converted_to_draft a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored if the chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. The assumevalid block will also be missing from the block index because the assumevalid block is set to match the `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a result, `reindex` can take significantly longe
...
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored if the chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. The assumevalid block will also be missing from the block index because the assumevalid block is set to match the `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a result, `reindex` can take significantly longe
...
🤔 stickies-v reviewed a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3448629035)
Approach ACK 86958ee668a950a03d08ef188f2de27137e89b33
I think the commit can be broken up a bit further, e.g. validationstate logic and chainman logic can be separate I think?
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3448629035)
Approach ACK 86958ee668a950a03d08ef188f2de27137e89b33
I think the commit can be broken up a bit further, e.g. validationstate logic and chainman logic can be separate I think?
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514666155)
> header is not owned and depends on the lifetime of the block
I don't think that's correct?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514666155)
> header is not owned and depends on the lifetime of the block
I don't think that's correct?
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514692129)
Would be nice if the test tests the entire block header interface.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514692129)
Would be nice if the test tests the entire block header interface.
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514681291)
This is marked as resolved, but I don't think this is resolved yet? I agree to keep the `state` name.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514681291)
This is marked as resolved, but I don't think this is resolved yet? I agree to keep the `state` name.
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3517541201)
I tried cargo culting @ajtowns 's testing code to check that currently one can create (multisig) UTXOs with hybrid keys, but cannot spend them:
self.log.info("Restarting node with -permitbaremultisig=1")
self.restart_node(0, extra_args=["-permitbaremultisig=1"])
self.log.info('Creating hybrid txs is standard, but spending is non-standard')
address = self.wallet.get_address()
tx = self.wallet.create_self_transfer()['tx'] # Pick a random coin(base)
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3517541201)
I tried cargo culting @ajtowns 's testing code to check that currently one can create (multisig) UTXOs with hybrid keys, but cannot spend them:
self.log.info("Restarting node with -permitbaremultisig=1")
self.restart_node(0, extra_args=["-permitbaremultisig=1"])
self.log.info('Creating hybrid txs is standard, but spending is non-standard')
address = self.wallet.get_address()
tx = self.wallet.create_self_transfer()['tx'] # Pick a random coin(base)
...
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514733608)
in a6c9f19c7d523fd6c2573e47e1992448b76f1977
Note to self/reviewers: we can't assert that the feerate diagram's last value matches cached `txTotalSize`, because it converts from weight to vsize happens per-chunk. These chunk vsizes can be smaller than the summed individual vsizes of the transactions.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514733608)
in a6c9f19c7d523fd6c2573e47e1992448b76f1977
Note to self/reviewers: we can't assert that the feerate diagram's last value matches cached `txTotalSize`, because it converts from weight to vsize happens per-chunk. These chunk vsizes can be smaller than the summed individual vsizes of the transactions.
💬 jonatack commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2514752164)
> This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks.
Perhaps clarify that in the documentation, as those that do run over both Tor and I2P could conclude that they risk being fingerprinted in the same way as described here.
I could be wrong, but I don't see who would listen on both clearnet and Tor (or I2P) if they are concerned about privacy. ISTM those who do that are willingly providing a service to the n
...
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2514752164)
> This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks.
Perhaps clarify that in the documentation, as those that do run over both Tor and I2P could conclude that they risk being fingerprinted in the same way as described here.
I could be wrong, but I don't see who would listen on both clearnet and Tor (or I2P) if they are concerned about privacy. ISTM those who do that are willingly providing a service to the n
...