💬 fanquake commented on pull request "Update libmultiprocess subtree in 30.x branch":
(https://github.com/bitcoin/bitcoin/pull/33519#issuecomment-3406955326)
> should we wait a bit longer?
For anything in particular? There are backports that are blocked on this, so it seems more useful to pull this now, and unblock them. It can always be pulled again later if there is a reason to do so.
(https://github.com/bitcoin/bitcoin/pull/33519#issuecomment-3406955326)
> should we wait a bit longer?
For anything in particular? There are backports that are blocked on this, so it seems more useful to pull this now, and unblock them. It can always be pulled again later if there is a reason to do so.
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2432972842)
good catch thanks I'll update this arg with `const` here and in the lambda
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2432972842)
good catch thanks I'll update this arg with `const` here and in the lambda
👋 fanquake's pull request is ready for review: "ci: add Valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/33461)
(https://github.com/bitcoin/bitcoin/pull/33461)
💬 furszy commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2432988211)
only const? why not passing the ref too
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2432988211)
only const? why not passing the ref too
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2433006469)
oops thanks, updating...
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2433006469)
oops thanks, updating...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2433196004)
I think I can just drop this first commit entirely. We don't actually care to not set the coins we fetch as dirty.
In the happy path, all these coins will be spent immediately after `ConnectBlock`, so they will be set to dirty anyways.
In the unhappy path where the valid proof-of-work block is found to be invalid, the dirty coins we added will just cause the coins to be overwritten by the same data in the db at the next flush.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2433196004)
I think I can just drop this first commit entirely. We don't actually care to not set the coins we fetch as dirty.
In the happy path, all these coins will be spent immediately after `ConnectBlock`, so they will be set to dirty anyways.
In the unhappy path where the valid proof-of-work block is found to be invalid, the dirty coins we added will just cause the coins to be overwritten by the same data in the db at the next flush.
💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3407265710)
My Guix build:
```
aarch64
d542a4060802978bee153978d6bc08069a1cc4ad27a45f55fcad46ce1988fe98 guix-build-c09c045da4c4/output/aarch64-linux-gnu/SHA256SUMS.part
874f7b024f4b9284fa693e8b8d0209f808d80ed512b4f28706774cd04c3f6565 guix-build-c09c045da4c4/output/aarch64-linux-gnu/bitcoin-c09c045da4c4-aarch64-linux-gnu-debug.tar.gz
b371efca582cab23f182eea3c68bd264e3c720899284bf4689f7743b42e09bf2 guix-build-c09c045da4c4/output/aarch64-linux-gnu/bitcoin-c09c045da4c4-aarch64-linux-gnu.tar.gz
a46ae223
...
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3407265710)
My Guix build:
```
aarch64
d542a4060802978bee153978d6bc08069a1cc4ad27a45f55fcad46ce1988fe98 guix-build-c09c045da4c4/output/aarch64-linux-gnu/SHA256SUMS.part
874f7b024f4b9284fa693e8b8d0209f808d80ed512b4f28706774cd04c3f6565 guix-build-c09c045da4c4/output/aarch64-linux-gnu/bitcoin-c09c045da4c4-aarch64-linux-gnu-debug.tar.gz
b371efca582cab23f182eea3c68bd264e3c720899284bf4689f7743b42e09bf2 guix-build-c09c045da4c4/output/aarch64-linux-gnu/bitcoin-c09c045da4c4-aarch64-linux-gnu.tar.gz
a46ae223
...
💬 maflcko commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433256383)
Excluding a test that is expected to pass under valgrind (without even documenting why it is excluded) also makes the test less useful when run locally, assuming that it covers all tests
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433256383)
Excluding a test that is expected to pass under valgrind (without even documenting why it is excluded) also makes the test less useful when run locally, assuming that it covers all tests
💬 fanquake commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433277209)
Happy to document that it is excluded because it is very slow compared to all other tests. Also happy to re-enable it, if we are fine to add ~13 minutes of runtime to this job, for a single slow test (it'd be about on par with ASAN at that point, so I guess still reasonable).
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433277209)
Happy to document that it is excluded because it is very slow compared to all other tests. Also happy to re-enable it, if we are fine to add ~13 minutes of runtime to this job, for a single slow test (it'd be about on par with ASAN at that point, so I guess still reasonable).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433141523)
Is there a reason we are checking for a null hash and return `nullptr` instead of an assertion? I see the c++ wrapper is throwing a runtime error in case of a `nullptr` return value for this (through the `Handle` constructor).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433141523)
Is there a reason we are checking for a null hash and return `nullptr` instead of an assertion? I see the c++ wrapper is throwing a runtime error in case of a `nullptr` return value for this (through the `Handle` constructor).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433069372)
Here "the chain" might suggest the active chain, but the function appears to work regardless - it simply returns the parent block tree entry via the `pprev` pointer, even if the block is no longer in the active chain (assuming block tree entries remain valid for the chainstate manager's lifetime).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433069372)
Here "the chain" might suggest the active chain, but the function appears to work regardless - it simply returns the parent block tree entry via the `pprev` pointer, even if the block is no longer in the active chain (assuming block tree entries remain valid for the chainstate manager's lifetime).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433243223)
Since `CBlockIndex` already has a calculated hash, can we return a non-owned `btck_BlockHash` here? (like `btck_transaction_get_txid()` for example)
A related question: why `GetBlockHash()` method in `CBlockIndex` returns a copied hash (unlike `GetHash()` in `CTransaction`)?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433243223)
Since `CBlockIndex` already has a calculated hash, can we return a non-owned `btck_BlockHash` here? (like `btck_transaction_get_txid()` for example)
A related question: why `GetBlockHash()` method in `CBlockIndex` returns a copied hash (unlike `GetHash()` in `CTransaction`)?
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433161387)
```suggestion
typedef void (*btck_ValidationInterfacePowValidBlock)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockConnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
```
nit: for consistency.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433161387)
```suggestion
typedef void (*btck_ValidationInterfacePowValidBlock)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockConnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
```
nit: for consistency.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433200559)
```suggestion
* @brief Returns the block height where the transaction that created this coin was included in the active chain.
```
nit
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433200559)
```suggestion
* @brief Returns the block height where the transaction that created this coin was included in the active chain.
```
nit
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433285220)
Good to mention about the lifetime of the returned pointer in the doc; also for `btck_transaction_out_point_get_txid()` (it seems we have been explicit about this for the rest of the returned const ptrs in the api).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433285220)
Good to mention about the lifetime of the returned pointer in the doc; also for `btck_transaction_out_point_get_txid()` (it seems we have been explicit about this for the rest of the returned const ptrs in the api).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433255059)
```suggestion
/**
* Opaque data structure for holding a transaction out point.
*
* Holds the txid and output index it is pointing to.
*/
typedef struct btck_TransactionOutPoint btck_TransactionOutPoint;
```
nit: not sure of this one, but i see we are using `txid` everywhere else.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433255059)
```suggestion
/**
* Opaque data structure for holding a transaction out point.
*
* Holds the txid and output index it is pointing to.
*/
typedef struct btck_TransactionOutPoint btck_TransactionOutPoint;
```
nit: not sure of this one, but i see we are using `txid` everywhere else.
💬 glozow commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407451665)
Given the mini_miner_selection fuzz target is removed in #33629, it might be appropriate to close this? Thanks for working on this fwiw!
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407451665)
Given the mini_miner_selection fuzz target is removed in #33629, it might be appropriate to close this? Thanks for working on this fwiw!
✅ glozow closed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062)
(https://github.com/bitcoin/bitcoin/pull/33062)
💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3407457684)
There's not much activity on this PR and this refactors code that will be refactored again in #33629. I think it's best to close this. Please leave a comment if you think this is a mistake.
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3407457684)
There's not much activity on this PR and this refactors code that will be refactored again in #33629. I think it's best to close this. Please leave a comment if you think this is a mistake.
👍 hebasto approved a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3341520000)
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
My Guix build:
```
x86_64
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82 guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21
...
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3341520000)
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
My Guix build:
```
x86_64
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82 guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21
...