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

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1401672632)
in 9426d3e7e5c884cd9ed31f2569a38a6e3712cc2c

We do other upgrade procedures in `LoadWallet()` function and only if `result != DBErrors::LOAD_OK`
Do we want to do the same for `UpgradeToGlobalHDKey`?

💬 Fei-Zodiac commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1822330742)
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKEORRRTE7C2NAKPR4BNNS3YFW2BHAVCNFSM6AAAAAATD3FIMWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBTHAYTCNRQGM>

S3RK ***@***.***> 于2023年11月22日周三 16:32写道：

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/wallet/walletdb.cpp
> <https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1401672632>:
>
> > @@ -1063,6 +1067,9 @@ static DBErrors LoadDescriptorWalletRecords(CWallet*
...

💬 n1bor commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1822425942)
Running fine on:
Intel(R) Atom(TM) CPU C2338 @ 1.74GHz - 2 Cores and 4Gb RAM
Ubuntu 20.04.6 LTS

With 453 connections:
Bitcoin Core client v26.0.0rc2 - server 70016/Satoshi:26.0.0/

ipv4 ipv6 onion total block
in 282 111 50 443
out 3 2 5 10 2
total 285 113 55 453