💬 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.
💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789130504)
Thanks @stickies-v went with your suggestion as my previous idea was buggy again🔥
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789130504)
Thanks @stickies-v went with your suggestion as my previous idea was buggy again🔥
📝 hebasto opened a pull request: "build: Update `qt` package up to 5.15.11"
(https://github.com/bitcoin/bitcoin/pull/28769)
In the light of https://github.com/bitcoin/bitcoin/pull/28622, we probably have to patch Qt. It seems reasonable to update it up to the latest available version before doing that.
(https://github.com/bitcoin/bitcoin/pull/28769)
In the light of https://github.com/bitcoin/bitcoin/pull/28622, we probably have to patch Qt. It seems reasonable to update it up to the latest available version before doing that.
🤔 vasild reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1708290287)
Approach ACK c1612ea1f0
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1708290287)
Approach ACK c1612ea1f0
💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378928478)
Is the intention here to mimic `NODE_NETWORK_LIMITED_MIN_BLOCKS` from the C++ source code, which is `288`? `MIN_BLOCKS_TO_KEEP` is a different constant defined in both the C++ and Python code which happens to be also `288`. Should `NODE_NETWORK_LIMITED_MIN_BLOCKS` be defined in the Python code (equal to 288, but not necessary equal or tied to `MIN_BLOCKS_TO_KEEP`)?
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378928478)
Is the intention here to mimic `NODE_NETWORK_LIMITED_MIN_BLOCKS` from the C++ source code, which is `288`? `MIN_BLOCKS_TO_KEEP` is a different constant defined in both the C++ and Python code which happens to be also `288`. Should `NODE_NETWORK_LIMITED_MIN_BLOCKS` be defined in the Python code (equal to 288, but not necessary equal or tied to `MIN_BLOCKS_TO_KEEP`)?