💬 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/
...

💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(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.

💬 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
...

💬 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?

🤔 ajtowns reviewed a pull request: "refactor: P2P transport without serialize version and type"
(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.

💬 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
...

💬 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.

💬 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
...

💬 furszy commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1401415245)
Done as suggested

💬 furszy commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1401415167)
Done as suggested

💬 furszy commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#issuecomment-1821977377)
Updated per feedback, thanks for the review @Sjors.

> Here's a commit that batches the descriptor import for external signers: https://github.com/Sjors/bitcoin/commit/286883a68d24e3600d9e608ccdea55763c50dde1 (I had to make TopupWithDB() public)

Thanks. Pulled the commit and made two changes to it:
1) Instead of making `TopupWithDB()` public, made it protected. The `ExternalSignerScriptPubKeyMan` class is derived from `DescriptorScriptPubKeyMan`.
2) Removed the unimplemented `SetupDescrip
...

💬 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-1822050550)
> Yeah, my only (rather minor) reservation about `UNPARSEABLE<...>` is that I think of `<...>` as a way of indicating "stuff being pushed",

Yeah, I guess being able to scan and say `<...>` means data is going onto the stack makes sense.

> So maybe just `UNPARSEABLE(...)`?

Could do `UNPARSEABLE:04010203` or something (colon instead of brackets).

(Arguably, everything after the first OP_SUCCESS in a tapscript could also be considered trailing hex that shouldn't be decoded. Probably mo
...

💬 ajtowns commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#discussion_r1401481010)
Might be good to add a comment explaining why copying this is often a bad thing and should only be done explicitly? (I think it's because `m_parents` and `m_children` need to stay in sync with the container holding this entry -- if you leave the copy out of the container, those sets will become bad; if you put it back into the container, those sets will probably be wrong)

🤔 ajtowns reviewed a pull request: "refactor: Make CTxMemPoolEntry only explicitly copyable"
(https://github.com/bitcoin/bitcoin/pull/28903#pullrequestreview-1743536103)
ACK 705e3f1de00bf30d728addd52a790a139d948e32

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1401611762)
Thanks, will do in one of the follow-up pulls, or here if I have to re-touch, whichever happens earlier.

📝 maflcko converted_to_draft a pull request: "refactor: VectorWriter and SpanReader without nVersion"
(https://github.com/bitcoin/bitcoin/pull/28912)
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.

💬 hebasto commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#issuecomment-1822264312)
CI restarted.