💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378719331)
nit: (same above), given that this function is only for internal use for this module, and only used once, is it needed?
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378719331)
nit: (same above), given that this function is only for internal use for this module, and only used once, is it needed?
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378757410)
> Also, `GetSerializeSize` could drop the int param completely?
Looks like; but seems a bit much for this PR. See https://github.com/ajtowns/bitcoin/commits/202310-getserializesize
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378757410)
> Also, `GetSerializeSize` could drop the int param completely?
Looks like; but seems a bit much for this PR. See https://github.com/ajtowns/bitcoin/commits/202310-getserializesize
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378762942)
That ditto one sure doesn't... Removed them both.
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378762942)
That ditto one sure doesn't... Removed them both.
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378768625)
No idea; seems easier to review without additional changes, though?
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378768625)
No idea; seems easier to review without additional changes, though?
💬 fanquake commented on pull request "Fix crash on selecting "Mask values" in transaction view":
(https://github.com/bitcoin-core/gui/pull/774#issuecomment-1788929772)
Pulled this into a branch for 25.x backporting.
(https://github.com/bitcoin-core/gui/pull/774#issuecomment-1788929772)
Pulled this into a branch for 25.x backporting.
💬 fanquake commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788945972)
The `rpc_createmultisig.py` test is now failing in two other CI jobs and MSAN is unhappy:
```bash
==19201==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x563f069e85d0 in bcmp /msan/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:856:10
#1 0x563f0956bff5 in base_blob<256u>::Compare(base_blob<256u> const&) const src/./uint256.h:54:66
#2 0x563f0956bff5 in operator!=(base_blob<256u> const&, base_blob<256u> const&) src/./uint256.h:57
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788945972)
The `rpc_createmultisig.py` test is now failing in two other CI jobs and MSAN is unhappy:
```bash
==19201==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x563f069e85d0 in bcmp /msan/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:856:10
#1 0x563f0956bff5 in base_blob<256u>::Compare(base_blob<256u> const&) const src/./uint256.h:54:66
#2 0x563f0956bff5 in operator!=(base_blob<256u> const&, base_blob<256u> const&) src/./uint256.h:57
...
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378798478)
That's just me trying to save few characters. Will rename it to `past_net_limited`.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378798478)
That's just me trying to save few characters. Will rename it to `past_net_limited`.
📝 fanquake opened a pull request: "[25.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/28768)
Collecting backports for the 25.x branch. Currently:
* https://github.com/bitcoin-core/gui/pull/774
(https://github.com/bitcoin/bitcoin/pull/28768)
Collecting backports for the 25.x branch. Currently:
* https://github.com/bitcoin-core/gui/pull/774
💬 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.