💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1187440836)
Shouldn't use this switch-case, now that the type is an enum class?

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1187451150)
I think unreachable code can be `Assert(false)`?

💬 Sjors commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1538386300)
Ran the same commit again with `-debug=ipc` to get more useful log info. ([log](https://gist.github.com/Sjors/c2b227d5c7d210524b9ca7d78b93c2ed))

I also tried disabling the indexes to simplify the setup, but that didn't prevent the issue.

💬 mononaut commented on pull request "rpc: distinguish between vsize and sigop-adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/27591#issuecomment-1538391444)
Concept ACK

💬 fanquake commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1538393639)
Nice - Concept ACK

💬 fanquake commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1538394966)
cc @martinus

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1187478957)
I'll clarify what I meant here: you can copy-paste the result of `listdescriptors`, because this now uses `h` instead of `'`

👍 ryanofsky approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1416823725)
Code review ACK 3b34ac7465919b968795063995f6610a73aa2d29. Left some comments but this looks good as is and would not change unless necessary

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187459164)
In commit "refactor: Move functions to BlockManager methods" (1def580e80a1892e473324016db9a0f2ffec0c27)

re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1140490220

> Thank you for the suggestion, will split this up.

Seems like this could be marked resolved

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187472027)
In commit "refactor: Use BlockManager options for params" (0400fb7b55415867d9ff0aa88898920c60b4b2df)

Seem like you could squash this commit into the last commit and decrease review burden because the changes are mechanical and basically every line that changed here changed last commit. Not important though.

💬 Sjors commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1187491311)
Descriptors have always required parsers to handle both `h` and `'`, and I'm not aware of any that don't. For old BIP32 derivation parsing software I'm less sure about that.

💬 Sjors commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1538425147)
Thanks for the review!

The reason for not breaking legacy wallet RPC calls is that I'm assuming any code that uses legacy wallets is really old and nobody wants to touch it. On our side eventually we want to get rid of the legacy wallet - or at least freeze it - so there's no need to modernise it.

I updated the title, description and release note. Pushed the latter as a new commit to not lose ACKs.

💬 Sjors commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1187496219)
Added (plus "and similar")

🤔 darosior reviewed a pull request: "Switch hardened derivation marker to h"
(https://github.com/bitcoin/bitcoin/pull/26076#pullrequestreview-1416887139)
re-ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c

💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-1538431662)
I'll rebase soon(tm).

🚀 fanquake merged a pull request: "msvc: Cleanup after upgrading libsecp256k1 up to 0.3.0"
(https://github.com/bitcoin/bitcoin/pull/27580)

💬 pinheadmz commented on pull request "msvc: Cleanup after upgrading libsecp256k1 up to 0.3.0":
(https://github.com/bitcoin/bitcoin/pull/27580#issuecomment-1538433259)
ACK
I noticed when switching locally between old and new branches that some untracked files got left over:

```

--> git status
On branch master
Your branch is up to date with 'origin/master'.

Untracked files:
(use "git add <file>..." to include in what will be committed)
src/secp256k1/src/libsecp256k1-config.h
src/secp256k1/src/libsecp256k1-config.h.in
src/secp256k1/src/stamp-h1

nothing added to commit but untracked files present (use "git add" to track)
```

💬 pinheadmz commented on pull request "msvc: Cleanup after upgrading libsecp256k1 up to 0.3.0":
(https://github.com/bitcoin/bitcoin/pull/27580#issuecomment-1538447056)
Actually I still get the untracked files warning after merge -- am i doing something wrong? Or would it make any sense to add these to gitignore?

💬 Sjors commented on pull request "rpc: distinguish between vsize and sigop-adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/27591#issuecomment-1538451521)
Concept ACK on clarifying and returning both variants.

It would useful to peruse a few other projects to see how they use `getmempoolentry` and `getrawmempool`, to determine if this is a breaking change.

👍 ryanofsky approved a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1416874927)
Code review ACK 95744f9cf1f143b6449a5046a914557b3886e3a2.

I think this looks good in it's current form, but the PR would have less churn and be simpler overall if the last commit "refactor: Avoid needless chain type string conversions" (95744f9cf1f143b6449a5046a914557b3886e3a2) was done earlier and became the second commit before commit "refactor: Replace string chain name constants with ChainTypes" (ca13faf0cc8acbaf425ac69c783415465403b5a3)