🤔 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

💬 Oeunchamreun90 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1401779127)
Hello

💬 ryanofsky commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1401780527)
Thanks! That approach of being able to combine different params is clever and solves my problem, avoiding boilerplate from having to create different streams for each object type.

Maybe at some point it would be useful to support default parameter values, too, so parameters like CNetAddr::V2 and CAddress::V2_NETWORK could be assumed if not specified. For some serialization parameters it may be better to be explicit, but for others there could be downsides. For example if I hardcode V2 in mult
...

💬 maflcko 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_r1401788091)
commit message: This is not a refactor