💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1402865607)
I'm just saying what my opinion is; doesn't mean I'm necessarily right. What you've come up with looks pretty good to me though.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1402993598)
I'm going to drop this test, unless you know a way how to handle this better.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403006667)
You're right.

My fear with this is unexpected behavior for tx sender: e.g., you craft a "package" thinking parent always goes ahead, but then child gets ahead (potentially with the attacker's help) and dropped on the floor due to some policy. Something along this, but maybe I'm making it up.
Are these concerns at least semi-valid? @glozow

I can add "see whether a parent is in the set already" check, when looking at a child, if we think it's worth it.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403013326)
I think i'd rather drop it from `TryRemovingFromSet`. It's not that strong anymore anyway, doesn't help much and pretty obvious i think. Let me know if you think differently.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403014150)
took this, but generally i think we have these mistakes all over the code and it's fine :)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403018216)
You're right, i shall think how to make it better.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403018671)
no, it's legacy i guess. wondering why the compiler haven't found it.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403019080)
i think it's from the latter commits. can be dropped here.

🤔 Zemaya366 reviewed a pull request: "[25.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/28655#pullrequestreview-1745902980)
I need help

💬 stickies-v commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1403082895)
nvm, I don't think this is actually an improvement.

💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403130801)
This comment should be adjusted, since there is no `arith_uint256(const std::string& str)` constructor anymore.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403144851)
If you have good reasons to believe we need these events later, then keeping them is fine by me. But why do you think this?

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403149685)
Ah, that makes sense, can you add a comment? Something like:
```cpp
// Add spkm_man to m_spk_managers before calling any method
// that might access it.
const auto& spkm = m_spk_managers[id] = std::move(spkm_man);
```

💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403147287)
Nit: Put these extra tests in a separate commit?

💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403149918)
These same tests also exist in `uint256_tests`. What are these additionally testing now? Can they be removed?

💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403155732)
Yes, it is a separate commit?

💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1824104612)
> Since it comes with a memory cost, I wonder if the the performance gain we get for it can be measured. Does this speed up any benchmarks / IBD?

It's less of a gain and more just trying to revert the performance regression I introduced in #28017. I guess you are asking if it is a noticeable regression. I would think both the performance regression and the new memory cost from here are negligible and I can certainly also live with what is in master now.

💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403157275)
^^ I ran HEAD~2 by mistake when reviewing this.

💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403159073)
There are two files. `arith_uint256_tests.cpp`, which mostly tests the `arith*` class, and `uint256_tests.cpp`, which mostly tests the `uint*` class.

💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403160611)
I think it is fine to call construction from a string "string constructor", but may change if I have to re-touch again for other reasons.