🤔 m3dwards reviewed a pull request: "Enable HW-accelerated implementations of SHA256 for MSVC builds"
(https://github.com/bitcoin/bitcoin/pull/24773#pullrequestreview-1790921033)
Tested e94ae81 on Windows 11 with VS2022. CPU: i7-4870HQ and saw big speed improvement.

On master:

```
$ ./src/bench_bitcoin.exe -filter=SHA256D64.*

| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 11.57 | 86,459,102.90 | 0.0% | 0.01 | `SHA256D64_1024_AVX2 using the 'standard' SHA256 implementation`
| 11.89 | 84,074,406.67 |
...

💬 m3dwards commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1432730913)
NIT: From what I understand we are enabling the special CPU instructions using intrinsics and not inline assembly. This makes the comment a little misleading and makes the symbol unfortunately named.

Perhaps the symbol could be?

```suggestion
/* Define this symbol to enable CPU SHA Extensions */
#define USE_SHA_EXTENTIONS
```

or?

```suggestion
/* Define this symbol to enable CPU Extensions */
#define USE_INTRINSICS
```

📝 BrandonOdiwuor opened a pull request: "test: Add test case for speding bare multisig"
(https://github.com/bitcoin/bitcoin/pull/29120)
Fixes https://github.com/bitcoin/bitcoin/issues/29113

✅ maflcko closed a pull request: "RPC: Add universal options argument to listtransactions"
(https://github.com/bitcoin/bitcoin/pull/22807)

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1864535003)
@mononaut do you know if these nodes were connected via p2p encryption (BIP324)?

💬 maflcko commented on pull request "test: Add test case for speding bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1432759454)
why re-assign?

👋 ajtowns's pull request is ready for review: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039)

📝 shuoer86 opened a pull request: "test: fix typo"
(https://github.com/bitcoin/bitcoin/pull/29121)
fix a typo: `invald wallet` -> `invalid wallet`

💬 c0deright commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1864618020)
Let me know if I can help with debugging this any further.

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1790997039)
Found possible deadlock causes and made a test to exercise one of them: https://github.com/furszy/bitcoin-core/commit/20344b91a03259eb33652863700562f5ef198128. Feel free to pull it here.

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1432778465)
In 4d846b7b448:

This will produce a deadlock.
if the statement execution fails, the semaphore will remain taken forever. Blocking all future statements execution.
Same will occur on `ExecStatement`.

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1432833047)
This can also cause a deadlock when the statement execution fails.

The semaphore is not released after a `TxnBegin` failure which will block forever if the software continues and executes a single write or another `TxnBegin` attempt. Which could happen as `TxnBegin` failure could be temporary.

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1432814176)
Need to check if `TxnBegin()` was already called on this object prior to acquiring the semaphore. Otherwise two `TxnBegin()` calls will cause a deadblock. Can throw a logic exception error if it happens.

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432851243)
We are using either `subtract_fee_from_outputs` or `subtractFeeFromOutputs`, depending on which one is present. I think handling that logic in the `ParseOutputs` function is better. Since the options object is passed by reference there isn't any performance difference in passing a reference to the entire options object vs passing a single key.

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432851596)
Good catch!

💬 zzzzasaa11 commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1864684692)
`master cricket property surface tooth million wash nominee priority wolf turtle notice`,`TXNLWRknggM7aq6W8z1YentbuqGV2tuDJg`
USDT is frozen. I am angry. It is impossible to give USDT to Tether. If you have the ability, take it.

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432869408)
I agree, see my comment in the description. Ultimately, I think `FundTransaction` should take a set of inputs and a set of outputs and return a `CreatedTransactionResult`. #25273 and this PR get us incrementally closer to that, but I don't want to fully tackle that in this PR as it's a bit more complicated than it seems, and the primary goal of this PR is to create the `CRecipients` objects in a standard way across all the RPC calls and pass them to `FundTransaction`

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432881141)
Also a good catch, we should be using the `recipients` vector to get the size here.

📝 sr-gi opened a pull request: "test: adds outbound eviction functional tests, updated comment on ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122)
## Motivation

While checking the outbound eviction code I realized a case was not considered within the comments, which in turn made me realize we had no functional tests for the outbound eviction case (when I went to check/add the test case).

This PR updated the aforementioned comment and adds functional tests to cover the outbound eviction logic, in addition to the unit tests covered by `src/test/denialofservice_tests.cpp`.

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updated comment on ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1864741013)
While the tests seem to run fine, I noticed a warning that appears every now and then:

```
TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset by peer
```

I'm not sure if something unintended is happening under the hood. Disconnections are expected, but it's our node who is supposed to trigger them, so maybe I'm missing something.