💬 maflcko commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378806543)
Any reason to use adjusted time, when the tip update time is recorded as NodeClock?
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378806543)
Any reason to use adjusted time, when the tip update time is recorded as NodeClock?
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788967144)
re-ACK 1bbddaa060b1826466740f580a604eb3d87a28c0 🙍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 1bbddaa060b182646674
...
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788967144)
re-ACK 1bbddaa060b1826466740f580a604eb3d87a28c0 🙍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 1bbddaa060b182646674
...
🤔 glozow reviewed a pull request: "Improve peformance of CTransaction::HasWitness (28107 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1708237778)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1708237778)
Concept ACK
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378813401)
> I suspect it'd be nicer to have PushMessage(pfrom, msgMaker.Make(BLOCKTXN, BIP_XXX_TX_COMPRESSION(resp))) so that net_processing can turn compression on based on whether it's been negotiated or not
In that case, compression would be using ser params, so the witness flag would also have to be passed down via net_processing? So it seems better to set the witness flag in net processing for compact blocks and blocktxn?
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378813401)
> I suspect it'd be nicer to have PushMessage(pfrom, msgMaker.Make(BLOCKTXN, BIP_XXX_TX_COMPRESSION(resp))) so that net_processing can turn compression on based on whether it's been negotiated or not
In that case, compression would be using ser params, so the witness flag would also have to be passed down via net_processing? So it seems better to set the witness flag in net processing for compact blocks and blocktxn?
💬 maflcko commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378819836)
what about leaving this function as-is and making the two members `std::optional` instead, then add an early-return one-line happy-path for the case when both are not nullopt?
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378819836)
what about leaving this function as-is and making the two members `std::optional` instead, then add an early-return one-line happy-path for the case when both are not nullopt?
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788979172)
Rebased over #28503 and addressed comments from @maflcko
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788979172)
Rebased over #28503 and addressed comments from @maflcko
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788983278)
re-ACK ad132a1c4d592ca81f540637d5e7def76dd81c87 🕺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ad132a1c4d592ca81f54
...
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788983278)
re-ACK ad132a1c4d592ca81f540637d5e7def76dd81c87 🕺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ad132a1c4d592ca81f54
...
🤔 ismaelsadeeq reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1708087754)
Light Code review ACK e579bb98ba8977af284ba6914ffb2b1da7f34cdd
Non expert look at the changes here, there are no any external usage of `CTxMemPool` `mapTx.find`.
The changes here looks good to me
Just a question and nits below.
I see there are still some external usages of `mapTx` in the codebase like `mapTx.size`, `mapTx.get` and `mapTx.project`.
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1708087754)
Light Code review ACK e579bb98ba8977af284ba6914ffb2b1da7f34cdd
Non expert look at the changes here, there are no any external usage of `CTxMemPool` `mapTx.find`.
The changes here looks good to me
Just a question and nits below.
I see there are still some external usages of `mapTx` in the codebase like `mapTx.size`, `mapTx.get` and `mapTx.project`.
💬 ismaelsadeeq commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378721448)
why not just here? is there a reason why you are getting the iterator of the txid?
```suggestion
const CTxMemPoolEntry::Children& children = e.GetMemPoolChildrenConst();
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378721448)
why not just here? is there a reason why you are getting the iterator of the txid?
```suggestion
const CTxMemPoolEntry::Children& children = e.GetMemPoolChildrenConst();
```
💬 ismaelsadeeq commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378747401)
In 3c7543cc98efb8d5e38b8c18f2f184c9272cd092 " [refactor] remove access to mapTx in mempool_tests.cpp "
nit: In the commit message there is still `mapTx` access in `mempool_tests.cpp` `CheckSort` test
maybe the commit message should be
[refactor] use `CTxMemPool` helper method to get a transaction mempool iterator.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378747401)
In 3c7543cc98efb8d5e38b8c18f2f184c9272cd092 " [refactor] remove access to mapTx in mempool_tests.cpp "
nit: In the commit message there is still `mapTx` access in `mempool_tests.cpp` `CheckSort` test
maybe the commit message should be
[refactor] use `CTxMemPool` helper method to get a transaction mempool iterator.
💬 ismaelsadeeq commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378813358)
Are you assuming that the callers of `IsRBFOptIn` must pass a mempool transaction with the `Assert`?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1378813358)
Are you assuming that the callers of `IsRBFOptIn` must pass a mempool transaction with the `Assert`?
💬 ajtowns commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378835011)
Making them optional would add 16 bytes to every `CTransaction` struct, I think? This approach would also prevent them from being marked `const` in `CTransaction`. If having `m_witness_hash` be non-const was okay, you could default it to `uint256::ZERO` and set `m_witness_hash = ComputeWitnessHash()` in the constructor. I don't think that's ideal though.
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378835011)
Making them optional would add 16 bytes to every `CTransaction` struct, I think? This approach would also prevent them from being marked `const` in `CTransaction`. If having `m_witness_hash` be non-const was okay, you could default it to `uint256::ZERO` and set `m_witness_hash = ComputeWitnessHash()` in the constructor. I don't think that's ideal though.
💬 ajtowns commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789003630)
> ```shell
> #3 0x563f0956bff5 in CTransaction::HasWitness() const src/./primitives/transaction.h:370:33
> #4 0x563f0956bff5 in void SerializeTransaction<CHashWriter, CTransaction>(CTransaction const&, CHashWriter&) src/./primitives/transaction.h:265:16
> ```
If it's not obvious, the bug is that `m_witness_hash` is initialised by serializing the transaction with its witness, but in that case, `Serialize` calls `HasWitness()` to work out which format to use, which with this change relies
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789003630)
> ```shell
> #3 0x563f0956bff5 in CTransaction::HasWitness() const src/./primitives/transaction.h:370:33
> #4 0x563f0956bff5 in void SerializeTransaction<CHashWriter, CTransaction>(CTransaction const&, CHashWriter&) src/./primitives/transaction.h:265:16
> ```
If it's not obvious, the bug is that `m_witness_hash` is initialised by serializing the transaction with its witness, but in that case, `Serialize` calls `HasWitness()` to work out which format to use, which with this change relies
...
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378842023)
> Any reason to use adjusted time, when the tip update time is recorded as NodeClock?
Good point. The idea was to considerate the peers' time too, so the node has less chances of requesting a block at the limited peers threshold that does no longer exist in the other side. In other words, if the peer's time deviates slightly from the node time, the peer could have pruned the block at the verge of the threshold.
But we could achieve the same outcome by reducing the window size by one or two b
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378842023)
> Any reason to use adjusted time, when the tip update time is recorded as NodeClock?
Good point. The idea was to considerate the peers' time too, so the node has less chances of requesting a block at the limited peers threshold that does no longer exist in the other side. In other words, if the peer's time deviates slightly from the node time, the peer could have pruned the block at the verge of the threshold.
But we could achieve the same outcome by reducing the window size by one or two b
...
💬 instagibbs commented on pull request "Fuzz: Check individual and package transaction invariants":
(https://github.com/bitcoin/bitcoin/pull/28764#discussion_r1378847851)
taken, thanks
(https://github.com/bitcoin/bitcoin/pull/28764#discussion_r1378847851)
taken, thanks
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378860381)
Not clear -- you could have `BIP_XXX_TX_COMPRESSION` be a `BlockTransactionsSerParams` and have the logic for compression-or-not happen in `BlockTransactions::Serialize`, or it could be a separate formatter. In either case the `TX_WITH_WITNESS` flag could go in with the logic there still, I think.
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378860381)
Not clear -- you could have `BIP_XXX_TX_COMPRESSION` be a `BlockTransactionsSerParams` and have the logic for compression-or-not happen in `BlockTransactions::Serialize`, or it could be a separate formatter. In either case the `TX_WITH_WITNESS` flag could go in with the logic there still, I think.
💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789036887)
Added a `std::optional<bool>` to `CTransaction` as a cache for `HasWitness`.
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789036887)
Added a `std::optional<bool>` to `CTransaction` as a cache for `HasWitness`.
💬 stickies-v commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789076984)
I'm not a fan of the `std::optional<bool>` cache approach, it's an error-prone API imo (even if it's private and small). I think it makes more sense to just calculate the boolean at initialization and follow the same pattern as for `hash` and `m_witness_hash`? It has to be calculated for every instance anyway.
As e.g. in https://github.com/stickies-v/bitcoin/commit/2e69a6597c1a07b319e9cba4601474fbe0761f71
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789076984)
I'm not a fan of the `std::optional<bool>` cache approach, it's an error-prone API imo (even if it's private and small). I think it makes more sense to just calculate the boolean at initialization and follow the same pattern as for `hash` and `m_witness_hash`? It has to be calculated for every instance anyway.
As e.g. in https://github.com/stickies-v/bitcoin/commit/2e69a6597c1a07b319e9cba4601474fbe0761f71
💬 hebasto commented on issue "macos: dmg not working on macOS Sonoma (14.x)":
(https://github.com/bitcoin/bitcoin/issues/28767#issuecomment-1789091997)
Does your issue look like https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-548136595?
If not, please provide more details / screenshots etc.
(https://github.com/bitcoin/bitcoin/issues/28767#issuecomment-1789091997)
Does your issue look like https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-548136595?
If not, please provide more details / screenshots etc.
💬 maflcko commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378907084)
> In other words, if the peer's time deviates slightly from the node time
I don't think `GetAdjustedTime` offers this precision even. Also, it is not a per-peer offset, but a global one.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378907084)
> In other words, if the peer's time deviates slightly from the node time
I don't think `GetAdjustedTime` offers this precision even. Also, it is not a per-peer offset, but a global one.