🤔 pinheadmz reviewed a pull request: "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28154#pullrequestreview-1561373322)
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664210830)
@fanquake Would you mind removing the single use of `NOLINT` and pushing, just so that we can make sure the c-i still fails as expected after all the changes?

Assuming it does, LGTM either with or without addressing [this comment](https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069).

💬 theuni commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1664234300)
Concept ACK. I didn't review the includes changes in detail, but the move makes sense to me. And I'm always up for one-data-structure-per-header :)

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283423591)
Added in the current push.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664258585)
> Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?

Done.

👍 instagibbs approved a pull request: "util: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1555428522)
ACK 76fe2c304201646d63b1b01b397e18a99252a2d9

played around with some extensions to see where it could go, I think longer term we could use `GenTxid` for situations where both are acceptable, and `Wtxid/Txid` when only one type is, and slowly migrate towards that. f.e. `GenTxid` could have a `GetTypedHash` to convert to the other.

💬 instagibbs commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279668749)
Let's use `Txid` here as well

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283434519)
I originally did this to keep the stream reading within the try catch block, while changing as few lines in the implementation as possible. We could just return the string value though in the `ReadImpl` and then complete the data reading in the `Read` function.

📝 eriknylund opened a pull request: "test: verify spend from 999-of-999 taproot multisig wallet"
(https://github.com/bitcoin/bitcoin/pull/28212)
Verify that a 999-of-999 taproot multisig wallet is possible and can spend from it, and that neither a 1-of-1000 nor 999-of-1000 is allowed.

The tests will require some time to run. On my Mac M1 the new tests run in about 40 seconds.

Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:
- code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
- code -26: min relay fee not met

Fixes #28179

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1664282446)
> But it could produce a corrupt dump, which someone might rely on for their backup (even if they shouldn't).

I've changed it to be optional and enabled with the argument `-withinternalbdb`.

> Not sure if that was fixed in your later push, but in any case,

Yes, I fixed that

> I pushed another crash to the qa-assets PR.

This seems to be crash in BDB itself rather than in our parser.

💬 fanquake commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1664282498)
> Any reason not to add it to our configure directly rather than guix?

I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building `bitcoind.exe`), while also retaining our GCC patching in Guix. Anything else would potentially leave us with unwanted code from dependencies/the rest of the toolchain.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664291833)
From: https://cirrus-ci.com/task/6097246463197184

```bash
[100%] Building CXX object CMakeFiles/bitcoin-tidy-tests.dir/example_logprintf.cpp.o
/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:65:15: warning: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf]
LogPrintf("hello world!");
^
\n
/tmp/cirrus-ci-build/contrib/devtools/bitcoin-tidy/example_logprintf.cpp:22:68: note: expanded from ma
...

💬 fanquake commented on pull request "fuzz: addrman, avoid `ConsumeDeserializable` when possible":
(https://github.com/bitcoin/bitcoin/pull/27918#discussion_r1283451002)
It'd be good to avoid making unrelated changes like this (deleting the file ending newline).

🚀 fanquake merged a pull request: "fuzz: addrman, avoid `ConsumeDeserializable` when possible"
(https://github.com/bitcoin/bitcoin/pull/27918)

💬 furszy commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1283446866)
tiny nit: would be good to initialize the variable to nullptr and false.

🤔 furszy reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-1561461768)
Code ACK 3e93e6b7

💬 furszy commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1283443148)
tiny nit: would be good to add a jump line here.

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283455714)
I generally like your approach of keeping it as clean a move as possible, and in the other commits am okay with foregoing some slight improvements because of that. Here, though, I feel like we're making both the interface and implementation way too complex because of it, so personally I'd much rather make some more changes.

🚀 fanquake merged a pull request: "doc: Clarify -datacarriersize, add -datacarriersize=2 tests"
(https://github.com/bitcoin/bitcoin/pull/27832)

💬 fanquake commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1283470705)
> sed

🥲