🤔 sipa reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2497104576)
I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2497104576)
I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1881060842)
In commit "[txrequest] GetCandidatePeers"
Would it be possible to add an invocation/comparison with this function in the `src/test/fuzz/txrequest.cpp` fuzz test?
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1881060842)
In commit "[txrequest] GetCandidatePeers"
Would it be possible to add an invocation/comparison with this function in the `src/test/fuzz/txrequest.cpp` fuzz test?
💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569724440)
> I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?
That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet. So I think we need to store the announcers somewhere else to remember that information, remove candidates that fail to resolve th
...
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569724440)
> I wonder if it's possible to avoid keeping track of the announcers in `m_orphans`, as I think it should match `m_txrequest.GetCandidatePeers(orphan_tx)`?
That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet. So I think we need to store the announcers somewhere else to remember that information, remove candidates that fail to resolve th
...
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902108458)
Thanks, will consider if I need to retouch. Though I think `txCoinbase` is more clear than `block.vtx[0]` and limiting the scope isn't that important.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902108458)
Thanks, will consider if I need to retouch. Though I think `txCoinbase` is more clear than `block.vtx[0]` and limiting the scope isn't that important.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569731568)
> because I would expect `createNewBlock()` to create a new block, but apparently it creates a new block template.
Indeed, I could rename the interface method (different PR) to e.g. `newBlockTemplate()`. Though I'd rather avoid the code churn in my pile of PRs :-)
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569731568)
> because I would expect `createNewBlock()` to create a new block, but apparently it creates a new block template.
Indeed, I could rename the interface method (different PR) to e.g. `newBlockTemplate()`. Though I'd rather avoid the code churn in my pile of PRs :-)
🤔 tdb3 reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2529629807)
Approach ACK
Reviewed code changes.
Syncing from genesis on mainnet (because we're touching `CheckProofOfWorkImpl()`, I don't mind being paranoid, and it happens to be the genesis block anniversary today), so will report back (with ACK if all is well).
Looks like we could add `target` to `getblockchaininfo` and `getchainstates` as well (the top two commits on https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/ add target and associated tests). Could be left for follow up if desir
...
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2529629807)
Approach ACK
Reviewed code changes.
Syncing from genesis on mainnet (because we're touching `CheckProofOfWorkImpl()`, I don't mind being paranoid, and it happens to be the genesis block anniversary today), so will report back (with ACK if all is well).
Looks like we could add `target` to `getblockchaininfo` and `getchainstates` as well (the top two commits on https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/ add target and associated tests). Could be left for follow up if desir
...
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569749036)
> >
>
> That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet.
Right, of course!
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2569749036)
> >
>
> That's true immediately after receiving the orphan, but we delete the entries from `m_txrequest` once we've added this tx to the orphanage - `m_txrequest` only stores what we haven't successfully downloaded yet.
Right, of course!
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2569803135)
Thank you for the review @stickies-v,
Updated 6c3bc478ca343557d798e2b18b5f99c08079ba9a -> 8e86e77311a2c76fd34b4481f83205b2432b151a ([chainman_flush_destructor_0](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_0) -> [chainman_flush_destructor_1](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_0..chainman_flush_destructor_1))
* Addressed @stickies-v's [commen
...
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2569803135)
Thank you for the review @stickies-v,
Updated 6c3bc478ca343557d798e2b18b5f99c08079ba9a -> 8e86e77311a2c76fd34b4481f83205b2432b151a ([chainman_flush_destructor_0](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_0) -> [chainman_flush_destructor_1](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_0..chainman_flush_destructor_1))
* Addressed @stickies-v's [commen
...
🤔 furszy reviewed a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2529696656)
q: why not store the id belonging to the pubkey with the correct parity bit inside the `SigningProvider` arg at the point where such a structure is loaded? (which most likely happens inside `ParsePubkeyInner()`).
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2529696656)
q: why not store the id belonging to the pubkey with the correct parity bit inside the `SigningProvider` arg at the point where such a structure is loaded? (which most likely happens inside `ParsePubkeyInner()`).
💬 furszy commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1902153333)
Just as a fun fact: this function could be reduced to a single line:
```c++
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) :
arg.GetKey(m_pubkey.GetID(), key);
}
```
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1902153333)
Just as a fun fact: this function could be reduced to a single line:
```c++
bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
{
return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) :
arg.GetKey(m_pubkey.GetID(), key);
}
```
💬 tdb3 commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902173650)
We repeat this pattern throughout the file.
Unless I'm missing something, seems like a good opportunity to de-duplicate with something like:
```c++
CBlock CreateAndGetBlock(const node::BlockCreateOptions& options)
{
auto mining{MakeMining()};
BOOST_REQUIRE(mining);
std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
BOOST_REQUIRE(block_template);
return block_template->getBlock();
}
```
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902173650)
We repeat this pattern throughout the file.
Unless I'm missing something, seems like a good opportunity to de-duplicate with something like:
```c++
CBlock CreateAndGetBlock(const node::BlockCreateOptions& options)
{
auto mining{MakeMining()};
BOOST_REQUIRE(mining);
std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
BOOST_REQUIRE(block_template);
return block_template->getBlock();
}
```
👍 tdb3 approved a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2529729064)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
Nice updates. Left a minor non-blocking comment.
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2529729064)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
Nice updates. Left a minor non-blocking comment.
💬 laanwj commented on pull request "[28.x] 28.1 backports and final changes":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2569868533)
> Are these significant enough to warrant a rc3 or can we go straight to final?
The only change here that could trigger a new RC is the change in `src/rpc/mining.cpp`, which affects the release binary, but as that's contained to a test-only RPC i don't think it's worth rolling another rc for. The rest are test changes.
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2569868533)
> Are these significant enough to warrant a rc3 or can we go straight to final?
The only change here that could trigger a new RC is the change in `src/rpc/mining.cpp`, which affects the release binary, but as that's contained to a test-only RPC i don't think it's worth rolling another rc for. The rest are test changes.
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569873452)
Thank you for the review @mzumsande,
Updated f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> 182945e946ac6a92daad012d6579e5441b9641cc ([blockmanDB_4](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_4) -> [blockmanDB_5](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_4..blockmanDB_5))
* Addressed @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977), added documentatio
...
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569873452)
Thank you for the review @mzumsande,
Updated f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> 182945e946ac6a92daad012d6579e5441b9641cc ([blockmanDB_4](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_4) -> [blockmanDB_5](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_4..blockmanDB_5))
* Addressed @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977), added documentatio
...
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569874763)
Re https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377
> I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.
I wish there would be better consensus on what the best practices are. If somebody has reasoning for doing things differently, do share!
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2569874763)
Re https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377
> I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.
I wish there would be better consensus on what the best practices are. If somebody has reasoning for doing things differently, do share!
🤔 sipa reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2529697189)
Concept ACK, some nits/questions:
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2529697189)
Concept ACK, some nits/questions:
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902163221)
Since `State::COMPLETED` is the final state, it's possible to move this condition into the loop (`it->m_txhash == txhash && it->GetState() != State::COMPLETED`); hitting COMPLETED means we're done with that hash.
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902163221)
Since `State::COMPLETED` is the final state, it's possible to move this condition into the loop (`it->m_txhash == txhash && it->GetState() != State::COMPLETED`); hitting COMPLETED means we're done with that hash.
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902181326)
Nit: can use `contains(peer)` instead of `count(peer) > 0`.
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902181326)
Nit: can use `contains(peer)` instead of `count(peer) > 0`.
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902155109)
Should this attempt to deduplicate? Is it possible that we have both an txid and wtxid announcement from the same peer for the same transaction?
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902155109)
Should this attempt to deduplicate? Is it possible that we have both an txid and wtxid announcement from the same peer for the same transaction?
💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902153685)
Should this attempt to recurse, and give all ancestors? Not relevant for 1p1c, but in general, in both call sites that seems desirable (an announcement for an orphan should be seen as an announcement for all its ancestors, resolving an orphan means all ancestors can be considered received).
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902153685)
Should this attempt to recurse, and give all ancestors? Not relevant for 1p1c, but in general, in both call sites that seems desirable (an announcement for an orphan should be seen as an announcement for all its ancestors, resolving an orphan means all ancestors can be considered received).