💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478798725)
Sure, but the smallest change here that would fix the bug would be to upcast the `4` multiplier to 64 bits as mentioned in https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2474414992.
I don't mind doing some combination of partial migration and not just the smallest possible diff, but I mentioned the outliers, will let you decide which ones are relevant and which ones to do separately. I don't particularly care about the order.
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478798725)
Sure, but the smallest change here that would fix the bug would be to upcast the `4` multiplier to 64 bits as mentioned in https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2474414992.
I don't mind doing some combination of partial migration and not just the smallest possible diff, but I mentioned the outliers, will let you decide which ones are relevant and which ones to do separately. I don't particularly care about the order.
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3468964102)
> > the second peer can endlessly send CMPCTBLOCK's
>
> If we're concerned about that, seems better to fix it directly?
>
> ```diff
> --- a/src/net_processing.cpp
> +++ b/src/net_processing.cpp
> @@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
> while (range_flight.first != range_flight.second) {
> if (range_flight.first->second.first == pfrom.GetId()) {
> requested_block_from_this_peer = tr
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3468964102)
> > the second peer can endlessly send CMPCTBLOCK's
>
> If we're concerned about that, seems better to fix it directly?
>
> ```diff
> --- a/src/net_processing.cpp
> +++ b/src/net_processing.cpp
> @@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
> while (range_flight.first != range_flight.second) {
> if (range_flight.first->second.first == pfrom.GetId()) {
> requested_block_from_this_peer = tr
...
💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478801132)
https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390 explains it well, I can get behind both as long as we're consistent
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478801132)
https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390 explains it well, I can get behind both as long as we're consistent
💬 00w1 commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3468967896)
> > floppy profile pic in screenshot
>
> <img alt="Image" width="272" height="186" src="https://private-user-images.githubusercontent.com/8077169/507760105-88349a98-b1b5-432d-a2af-9a21cb5372ce.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjE4NDI1NDQsIm5iZiI6MTc2MTg0MjI0NCwicGF0aCI6Ii84MDc3MTY5LzUwNzc2MDEwNS04ODM0OWE5OC1iMWI1LTQzMmQtYTJhZi05YTIxY2I1MzcyY2UucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU
...
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3468967896)
> > floppy profile pic in screenshot
>
> <img alt="Image" width="272" height="186" src="https://private-user-images.githubusercontent.com/8077169/507760105-88349a98-b1b5-432d-a2af-9a21cb5372ce.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjE4NDI1NDQsIm5iZiI6MTc2MTg0MjI0NCwicGF0aCI6Ii84MDc3MTY5LzUwNzc2MDEwNS04ODM0OWE5OC1iMWI1LTQzMmQtYTJhZi05YTIxY2I1MzcyY2UucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU
...
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478816900)
> These two are used in consensus code as well,
No, they are not. Only one of them is used in psbt, which is not consensus.
One way to check if a function is used somewhere is to `=delete` it and compile.
The following diff compiles:
```diff
diff --git a/src/psbt.h b/src/psbt.h
index f0de079f7b..ec235ed2fc 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -212,7 +212,7 @@ void DeserializeMuSig2ParticipantPubkeys(Stream& s, SpanReader& skey, std::map<C
std::vector<unsigned char>
...
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478816900)
> These two are used in consensus code as well,
No, they are not. Only one of them is used in psbt, which is not consensus.
One way to check if a function is used somewhere is to `=delete` it and compile.
The following diff compiles:
```diff
diff --git a/src/psbt.h b/src/psbt.h
index f0de079f7b..ec235ed2fc 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -212,7 +212,7 @@ void DeserializeMuSig2ParticipantPubkeys(Stream& s, SpanReader& skey, std::map<C
std::vector<unsigned char>
...
💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468999171)
(force pushed for Alpine)
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468999171)
(force pushed for Alpine)
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478845769)
Dropped the `blockstorage.cpp` changes
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478845769)
Dropped the `blockstorage.cpp` changes
✅ glozow closed an issue: "Remove *petertodd.net DNS seed for testnet and mainnet"
(https://github.com/bitcoin/bitcoin/issues/33736)
(https://github.com/bitcoin/bitcoin/issues/33736)
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478861359)
fixed
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478861359)
fixed
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478862391)
done, thanks
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478862391)
done, thanks
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3469069615)
[ab8bc7a](https://github.com/bitcoin/bitcoin/commit/ab8bc7a1f877ad9e80a112dae6a12ff6ddbce076) to [48d1151](https://github.com/bitcoin/bitcoin/commit/48d11516da6351358080493fa95ea6252ce08525):
Rebased, addressed remaining feedback, including resetting `m_cached_finished_ibd` reset to cleanup. Moved out of draft.
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3469069615)
[ab8bc7a](https://github.com/bitcoin/bitcoin/commit/ab8bc7a1f877ad9e80a112dae6a12ff6ddbce076) to [48d1151](https://github.com/bitcoin/bitcoin/commit/48d11516da6351358080493fa95ea6252ce08525):
Rebased, addressed remaining feedback, including resetting `m_cached_finished_ibd` reset to cleanup. Moved out of draft.
👋 mzumsande's pull request is ready for review: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533)
(https://github.com/bitcoin/bitcoin/pull/31533)
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478881137)
from the llm:
ReceivedBlockTransaction -> ReceivedBlockTransactions [the comment refers to the ReceivedBlockTransactions function; singular "ReceivedBlockTransaction" is incorrect and may confuse readers]
making no assumptions what must -> making no assumptions about what must [missing preposition "about" makes the phrase ungrammatical and harder to parse]
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478881137)
from the llm:
ReceivedBlockTransaction -> ReceivedBlockTransactions [the comment refers to the ReceivedBlockTransactions function; singular "ReceivedBlockTransaction" is incorrect and may confuse readers]
making no assumptions what must -> making no assumptions about what must [missing preposition "about" makes the phrase ungrammatical and harder to parse]
🤔 ajtowns requested changes to a pull request: "RPC/txoutproof: Support including (and verifying) proofs of wtxid"
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-3400680165)
Transaction proofs are an interoperability feature, I don't think it makes much sense to implement this without also documenting it and providing test vectors via a BIP.
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-3400680165)
Transaction proofs are an interoperability feature, I don't think it makes much sense to implement this without also documenting it and providing test vectors via a BIP.
💬 ajtowns commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478870174)
I think this should probably be automatic, with the current "array of txids" result being stuck behind a `-deprecatedrpc` option.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478870174)
I think this should probably be automatic, with the current "array of txids" result being stuck behind a `-deprecatedrpc` option.
💬 ajtowns commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478893275)
Being able to easily prove/verify that a tx was included in a reorged block seems fine?
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478893275)
Being able to easily prove/verify that a tx was included in a reorged block seems fine?
💬 ajtowns commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478855640)
There is no such subclass?
I think all this logic should entirely be in a separate class though. For efficiency probably two paths:
* if the generation tx has no witness commitment; provde all (w)txids and the generation tx via the block partial merkle tree.
* if the generation tx does have a witness commitment; provide just the generation tx's merkle path back to the block header, and prove all wtxids via a partial merkle tree back to the witness commitment
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478855640)
There is no such subclass?
I think all this logic should entirely be in a separate class though. For efficiency probably two paths:
* if the generation tx has no witness commitment; provde all (w)txids and the generation tx via the block partial merkle tree.
* if the generation tx does have a witness commitment; provide just the generation tx's merkle path back to the block header, and prove all wtxids via a partial merkle tree back to the witness commitment
💬 ajtowns commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478889737)
I would have expected this logic to be in merkleblock.cpp as a standard part of verifying a proof (eg so that it could be included in kernel), not in the RPC code.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478889737)
I would have expected this logic to be in merkleblock.cpp as a standard part of verifying a proof (eg so that it could be included in kernel), not in the RPC code.
💬 ajtowns commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478880286)
This information (height, confirmations, assumeutxo info) seems better obtained via `getblockheader` on the blockhash to me.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2478880286)
This information (height, confirmations, assumeutxo info) seems better obtained via `getblockheader` on the blockhash to me.
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478901830)
fixed those.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2478901830)
fixed those.