💬 glozow commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1400876966)
Could this use the `Wtxid` type?

📝 maflcko opened a pull request: " refactor: Remove unused and fragile string interface from arith_uint256 "
(https://github.com/bitcoin/bitcoin/pull/28924)
The string interface (`base_uint(const std::string&)`, as well as `base_uint::SetHex`) is problematic for many reasons:

* It is unused (except in test-only code).
* It is redundant with the `uint256` string interface: `std::string -> uint256 -> UintToArith256`.
* It is brittle, because it inherits the brittle `uint256` string interface, which is brittle due to the use of `c_str()` (embedded null will be treated as end-of string), etc ...

Instead of fixing the interface, remove it since i
...

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1400824727)
no, I don't know a better approach, but thanks for the explanation!

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1401009634)
The test fails in debug mode because this assume conflicts with the test saying "Adding a duplicate transaction should not happen, but it does happen, nothing should break.".

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1401014445)
Although this reduced it, I think the situation where the child is fanouted ahead could still happen if we receive the parent first, add it to the recon set, and only after that receive the child and decide to fanout it.
Not sure if that is a problem though.

💬 ryanofsky commented on pull request "multiprocess: Add basic type conversion hooks":
(https://github.com/bitcoin/bitcoin/pull/28921#issuecomment-1821552816)
Updated 118d60e95cc5c8c0d5a88c0d16904355c7e0c5f6 -> 33d796411dd4d8098287e1307f1337b6e2eab3e7 ([`pr/ipcc.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipcc.2) -> [`pr/ipcc.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipcc.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipcc.2..pr/ipcc.3)) replacing `CDataStream` with `DataStream` as suggested

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1821568461)
Rebased 18d382f4f8a74612ae0e91efd8a2b83bb665e081 -> 87e3a3b694abc4e0cf8241e31905d3f604c611cf ([`pr/ipc.201`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.201) -> [`pr/ipc.202`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.202), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc.201-rebase..pr/ipc.202)) on top of base PR #28921, also fixing conflicts with #28438 and #28503

🤔 sr-gi reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1737983905)
Concept ACK

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399472433)
nit: *the* reconciliation set

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1397919287)
Given the docs on `{Pre}RegisterPeer` and the last paragraph on `TryRemovingFromSet`, shouldn't this also advise the caller to make sure the peer is registered?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399699565)
Is this being used?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399729409)
The TODOs for the pubic fields referring to `m_we_initiate` and `m_k0` should be fixable at this point, given they are both used by `GetFanoutTargets`, shouldn't they?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399659166)
I think this is a bit counter-intuitive. If the transaction to be added is already in the set, we will treat this as if it was added when that is actually not the case.

I guess the reasoning is that there should be no way of adding the same transaction more than once, so `false` is being used to signal failures. However, this seems like a way to potentially shoot ourselves in the foot.

If we want to keep the logic as is, I'd suggest that we at least mention this in the functions docs, giv
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399703593)
I'm guessing this is supposed to be **integral**\_part and fractional\_**part** (?)

source: https://en.cppreference.com/w/cpp/numeric/math/modf

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399709981)
Shouldn't this need to be the other way around?

Maybe I'm confused by the name of the variables, but it seems like if `add_extra` is true, then you should do `x+1`

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399750369)
nit: also, `tagets_size` may be a better name, in lieu of `limit` which is already being used, given this does not really refer to the `targets` themselves, but to the size of the returned `targets` collection

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1400838979)
Isn't `reconciles_txs` only used within `fSendTrickle == true`? Wouldn't it be worth moving it to that context?

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1401352213)
I think I may have miss read the [initial comment](https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219668280) but I have updated the message and the tests.

I am now testing the whole object we get back from `self.nodes[0].getprioritisedtransactions()` so in the future if there are new transactions added this assertion will fail

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1821889243)
looks like the "test each commit" git action failed but doesn't seems related to the changes proposed

I see the following in the logs
```
E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/a/avahi/libavahi-core7_0.8-5ubuntu5.1_amd64.deb 404 Not Found [IP: 52.252.75.106 80]
E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/a/avahi/avahi-daemon_0.8-5ubuntu5.1_amd64.deb 404 Not Found [IP: 52.252.75.106 80]
```

💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401318588)
I didn't drop the spkm arg on purpose. It was a design decision. The idea is the following one:
The wallet stores multiple spkms, and it registers to multiple signals from each of them. If we don't provide the pointer to the originator spkm, we will not be able to distinguish them. And, I'm pretty confident we will need this data, or need to access the origin spkm during an event, sooner or later.

But if you are strong on this, I could change it. We could always re-add the spkm pointer when/
...