💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478875833)
I see that you don't change the hardcoded value `0xc0`, just pass it with function calls.
It does make sense to generalize `taproot_tree_helper` and `taproot_construct`.

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478878383)
I'm not familiar with taproot, but do you think it does make sense to add a test to check if someone is passing the correct version constants to the functions?

📝 brunoerg opened a pull request: "fuzz: remove unused `args` and `context` from `FuzzedWallet`"
(https://github.com/bitcoin/bitcoin/pull/29388)
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them.

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1928047502)
@sipa Huh, thanks.

I could've sworn I saw an autoconf check for that that defined it when I was grepping around last week, but I guess I got it mixed up in my head with another. The lack of `HAVE_` or `USE_` should've been a giveaway :)

💬 brunoerg commented on pull request "fuzz: remove unused `args` and `context` from `FuzzedWallet`":
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1928050767)
cc: @maflcko

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1928054768)
Aha, it used to be an autoconf check: #23761

That explains the zombie include.

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478936428)
I think you probably need to understand Taproot at least a bit to understand why we aren't changing the hard coded constant `0xc0` for now. I would recommend taking a look at #29221 to understand what a new using a _new_ leaf version looks like.

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1478916123)
This is an extreme edge case, but unlocking the semaphore prior to refreshing the connection could theoretically allow another thread to dump this information to disk. To be 100% sure that no race will ever happen, this line should be after the connection refreshing process (after line 447).

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1863823000)
Code reviewed, looking good. Left a last comment.

Also, have slightly modified my previous unit test to validate the changes (ba333a239b8aec4a7a6a79d11d4668730ee24803).
It is not really needed but feel free to take it if you like it (conceptually, it is similar to your test, the diff is that it is verifying the writes execution order and checks that when a batch finishes writing data to disk, another batch in a concurrent process can read the data immediately).

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478943273)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888

Thanks, restored this line

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1863598337)
Updated a3228f02f69e28960f7da76054965c916e781f27 -> 3bdbc3fc7f116856acc7d955f591110b96f5ebd2 ([`pr/nofake.2`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.2) -> [`pr/nofake.3`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.2..pr/nofake.3)) implementing all suggestions, adding test coverage for the RPCs, and adding a new commit that drops the no longer needed BLOCK_ASSUMED_VALID flag.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766159)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772

Thanks, removed this

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478757569)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490

Added a comment. It is necessary to set nSequenceId to avoid an assert failure in CheckBlockIndex. I think it's also a reasonable thing to do to make the block look like it was never received.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478776687)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900

Thanks, reverted this

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478947294)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794

Restored this check

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766291)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941

> Same (sequence id)

Thanks, removed this

⚠️ ThunderCat1000 opened an issue: "Monkey Test Feature (MTF)"
(https://github.com/bitcoin/bitcoin/issues/29389)
### Please describe the feature you'd like to see added.

Consider the following scenario:

Say Chad has a Bitcoin wallet and wants to send a large amount of Bitcoins to another wallet. Chad decides to send a smaller test amount first to make sure everything is OK. Chad proceeds to sign and send the transaction, however, the software has two bugs that transmit his entire balance to a wallet that is not under Chads control. Wouldn't it be nice to be able to perform a formal test (what i call a
...

👍 jarolrod approved a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1863989093)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513

💬 achow101 commented on issue "Monkey Test Feature (MTF)":
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928516796)
I don't see how this is at all useful. If you are concerned about the existence of such a bug, then you can make the test transactions yourself. However I don't see how such test transactions would safeguard the sender anyways - if there exists a bug that can send all funds in the wallet to someone else, then making a test transaction could hit that bug and send all funds regardless.

A better method is to split up the steps of unsigned transaction creation, transaction signing, and transactio
...

📝 theStack opened a pull request: "test: speedup bip324_crypto.py unit test"
(https://github.com/bitcoin/bitcoin/pull/29390)
Executing the unit tests for the bip324_crypto.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`:

https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L193-L194
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/cr
...

💬 ThunderCat1000 commented on issue "Monkey Test Feature (MTF)":
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928583380)
The idea was that there can exist 2 bugs that work together to deplete your wallet when you are merely sending a dust transaction using a normal wallet. A standard feature for wallets to test one part of the transaction in isolation (the send to address function) without being impacted by other code that has a dynamic amount for the transaction, would give peace of mind without the complexity of the process you are suggesting. Hence the name "Monkey Test"