💬 ryanofsky commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1402853197)
I guess the idea of having default parameters is off the table then. So probably the way forward to make IPC code safer is have an IPC serialization parameters like your suggestion.
For now though I left the safety issue aside and implemented a way for IPC code to pass multiple stream parameters to ParamsStream in #28929 (used in #10102 [here](https://github.com/ryanofsky/bitcoin/blob/7ba123f1b47a4a790370c9a5975b1e0b4026a241/src/ipc/capnp/common-types.h#L52-L56)), so the same stream can be us
...
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1402853197)
I guess the idea of having default parameters is off the table then. So probably the way forward to make IPC code safer is have an IPC serialization parameters like your suggestion.
For now though I left the safety issue aside and implemented a way for IPC code to pass multiple stream parameters to ParamsStream in #28929 (used in #10102 [here](https://github.com/ryanofsky/bitcoin/blob/7ba123f1b47a4a790370c9a5975b1e0b4026a241/src/ipc/capnp/common-types.h#L52-L56)), so the same stream can be us
...
🤔 ajtowns reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1745623209)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1745623209)
Concept ACK
💬 ajtowns commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1402864403)
Dropping `SERIALIZE_METHODS_PARAMS` doesn't seem like an improvement?
I would have expected something more like:
```c++
// this pr: #define SER_PARAMS(type) (s.template GetParams<type>())
// instead:
template<typename T, typename Stream>
const auto& GetSerParams(const Stream& s)
{
return s.template GetParams<T>();
}
// FORMATTER_METHODS_PARAMS has the same signature, but now uses the new GetSerParams<>()
#define FORMATTER_METHODS_PARAMS(cls, obj, paramcls, paramobj) \
t
...
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1402864403)
Dropping `SERIALIZE_METHODS_PARAMS` doesn't seem like an improvement?
I would have expected something more like:
```c++
// this pr: #define SER_PARAMS(type) (s.template GetParams<type>())
// instead:
template<typename T, typename Stream>
const auto& GetSerParams(const Stream& s)
{
return s.template GetParams<T>();
}
// FORMATTER_METHODS_PARAMS has the same signature, but now uses the new GetSerParams<>()
#define FORMATTER_METHODS_PARAMS(cls, obj, paramcls, paramobj) \
t
...
💬 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.
(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.
(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.
(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.
(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 :)
(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.
(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.
(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.
(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
(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.
(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.
(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?
(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);
```
(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?
(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?
(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?
(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.
(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.