💬 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

🥲

💬 paplorinc commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283470743)
won't this pass if no exception is raised?
Shouldn't we add something like either:
```Python
try:
self.do_test_k_of_n(1, 1000)
assert False, "Expected exception was not raised for 1-of-1000 test"
except JSONRPCException as e:
```

or via unittest:
```Python
# Test 1-of-1000, should not succeed
with self.assertRaises(JSONRPCException) as cm:
self.do_test_k_of_n(1, 1000)
self.assertEqual(cm.exception.error["code"], -5)
self.assertIn("Cannot have 1000 keys in multi_a; mus
...

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

Nice, thanks!

💬 fanquake commented on pull request "build: Bump minimum supported Clang to clang-13":
(https://github.com/bitcoin/bitcoin/pull/28210#issuecomment-1664317915)
> This fails on macOS 11.0,

Yea, this'll at least need to wait for the macOS compiler bump, but the next time that happens (soon) it will be from 11 to 16, if not 17.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664319251)
Dropped the test commit back out.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283479802)
Thanks for pointing this out 🙏 I agree it shouldn't be possible for this test to succeed so an exception should be raised.

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664326798)
Concept ACK

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664327227)
Concept ACK