💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1399472433)
nit: *the* reconciliation set
(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?
(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?
(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?
(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
...
(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
(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`
(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
(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?
(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
(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]
```
(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/
...
(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/
...
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401377409)
sure, updated.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401377409)
sure, updated.
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401323753)
I think I had a birth time per descriptor before. This work was taken from my bip157/bip158 light client branch. Where I only use the filters that have a birth time below the header time to decrease the false-positive rate.
I was thinking on porting it to the wallet rescan process when I did it. But.. on that time, I decided to leave it for when the p2p related PRs get merged. And.. #28170, #28120 and #27837 are still open.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401323753)
I think I had a birth time per descriptor before. This work was taken from my bip157/bip158 light client branch. Where I only use the filters that have a birth time below the header time to decrease the false-positive rate.
I was thinking on porting it to the wallet rescan process when I did it. But.. on that time, I decided to leave it for when the p2p related PRs get merged. And.. #28170, #28120 and #27837 are still open.
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1401380212)
The approach we've got looks like:
```c++
dumb_serializer << SER_PARAMS(object)
// file, CTransaction
```
which then gets translated to:
```c++
dumb_serializer << smart_object
// file, ParamsWrapper<TransactionSerParams, CTransaction>
```
then
```c++
smart_serializer.Serialize(object, SER_PARAMS)
// ParamsStream<TransactionSerParams, dumb_serializer>, CTransaction
```
The advantage of saying `TX_WITH_WITNESS` everywhere is that way we're being e
...
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1401380212)
The approach we've got looks like:
```c++
dumb_serializer << SER_PARAMS(object)
// file, CTransaction
```
which then gets translated to:
```c++
dumb_serializer << smart_object
// file, ParamsWrapper<TransactionSerParams, CTransaction>
```
then
```c++
smart_serializer.Serialize(object, SER_PARAMS)
// ParamsStream<TransactionSerParams, dumb_serializer>, CTransaction
```
The advantage of saying `TX_WITH_WITNESS` everywhere is that way we're being e
...
💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1400147412)
Maybe add `explicit` to this?
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1400147412)
Maybe add `explicit` to this?
🤔 ajtowns reviewed a pull request: "refactor: P2P transport without serialize version and type"
(https://github.com/bitcoin/bitcoin/pull/28892#pullrequestreview-1741304017)
ACK fafd5e5becf035426a45179ff592c11ccf2d5da5
(https://github.com/bitcoin/bitcoin/pull/28892#pullrequestreview-1741304017)
ACK fafd5e5becf035426a45179ff592c11ccf2d5da5
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1821925803)
Updated per feedback, thanks @Sjors.
> You could also add a oldest_timestamp (or birth) field to getwalletinfo which would contain the birth date.
Sounds good 👌🏼.
Added it in another commit to not disable the possibility of verifying the failure by cherry-picking the test commit on top of master.
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1821925803)
Updated per feedback, thanks @Sjors.
> You could also add a oldest_timestamp (or birth) field to getwalletinfo which would contain the birth date.
Sounds good 👌🏼.
Added it in another commit to not disable the possibility of verifying the failure by cherry-picking the test commit on top of master.
💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1821929092)
> Brainstormy idea for undecodable scripts.
>
> These are necessarily pushes (direct or OP_PUSHDATA[124]) whose prescribed length exceeds the end of the script. What about using `<...+N>` or `OPCODE<...+N>`, where `...` is hexadecimal as before, and `N` is a decimal number indicating how many bytes are missing?
>
> EDIT: for `OP_PUSHDATA[124]` it's also possible that the 1-4 byte length field itself straddles the script end, and that isn't covered by this suggestion.
If you're adding ex
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1821929092)
> Brainstormy idea for undecodable scripts.
>
> These are necessarily pushes (direct or OP_PUSHDATA[124]) whose prescribed length exceeds the end of the script. What about using `<...+N>` or `OPCODE<...+N>`, where `...` is hexadecimal as before, and `N` is a decimal number indicating how many bytes are missing?
>
> EDIT: for `OP_PUSHDATA[124]` it's also possible that the 1-4 byte length field itself straddles the script end, and that isn't covered by this suggestion.
If you're adding ex
...
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401385547)
> [4bfb243](https://github.com/bitcoin/bitcoin/commit/4bfb243ac41cf1396cf55f8b24de2ba52186c7a4): this seems simpler:
>
> ```c++
> TryUpdateBirthTime(spkm_man->GetTimeFirstKey());
> m_spk_managers[id] = std::move(spkm_man);
> ```
That could introduce a bug in the future if we ever add logic inside `TryUpdateBirthTime()` accessing `m_spk_managers` in some way.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401385547)
> [4bfb243](https://github.com/bitcoin/bitcoin/commit/4bfb243ac41cf1396cf55f8b24de2ba52186c7a4): this seems simpler:
>
> ```c++
> TryUpdateBirthTime(spkm_man->GetTimeFirstKey());
> m_spk_managers[id] = std::move(spkm_man);
> ```
That could introduce a bug in the future if we ever add logic inside `TryUpdateBirthTime()` accessing `m_spk_managers` in some way.
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1821933197)
Yeah, my only (rather minor) reservation about `UNPARSEABLE<...>` is that I think of `<...>` as a way of indicating "stuff being pushed", which wouldn't be the case here. And if we're going to introduce some special syntax for unparseable stuff anyway, I thought it might be useful to integrate *into* the push syntax itself (as everything unparseable is an incomplete push).
That said, I agree that the +N thing isn't super readabable, and it's incomplete anyway (can't deal with OP_PUSHDATA[124] w
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1821933197)
Yeah, my only (rather minor) reservation about `UNPARSEABLE<...>` is that I think of `<...>` as a way of indicating "stuff being pushed", which wouldn't be the case here. And if we're going to introduce some special syntax for unparseable stuff anyway, I thought it might be useful to integrate *into* the push syntax itself (as everything unparseable is an incomplete push).
That said, I agree that the +N thing isn't super readabable, and it's incomplete anyway (can't deal with OP_PUSHDATA[124] w
...