👍 theStack approved a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1795584001)
ACK ad0ae3d2128d04ff4f62a4bf612286d153dc314b

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1435840695)
nit: IIUC, `b""` is also returned if the decrypted package content is empty. As call-sites currently use
the length integer return value (0) to check for the "only part of packet is received" condition, this isn't a problem, but maybe the description can be improved to avoid confusion.

💬 1440000bytes commented on pull request "Compressed Bitcoin Transactions":
(https://github.com/bitcoin/bitcoin/pull/29134#issuecomment-1868547068)
Concept ACK

25-50% savings look good and compressed raw transactions can help in some cases as mentioned in the PR description.

Related Q&A: https://bitcoin.stackexchange.com/questions/98886/compress-transaction-hex-string (still wont fit in one text message but will need less text messages)

🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1795601906)
Sorry for the delay @naumenkogs. 've been on #28170 before returning here. All good now, it is ready to go.

> The code looks alright, but i'd still rather have [this](https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930) addressed first. It looks like even #28170 doesn't directly handle this, but rather prepare the codebase for another PR handling it?

The concern was:
All connections are from limited peers, we are lagging behind and we no longer disconnect from them
...

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1435850669)
Thanks updated

💬 fjahr commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1868564680)
> If we are going to change it here, should also switch in `hash.cpp`

Done, thanks! I grepped for it didn't find this one, probably because of the all-caps.

💬 GregTonoski commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1868893820)
Yes, the exact prefix in the file is:
```
##
## bitcoin.conf configuration file.
## Generated by contrib/devtools/gen-bitcoin-conf.sh.
##
## Lines beginning with # are comments.
## All possible configuration options are provided. To use, copy this file
## to your data directory (default or specified by -datadir), uncomment
## options you would like to change, and save the file.
##
```

I would suggest renaming the file, e.g. "example-bitcoin.conf".

💬 dimitaracev commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868965467)
I can take a look at this.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1436080062)
>Alternatively, could use util::Result failure values after https://github.com/bitcoin/bitcoin/pull/25665 is merged.

will resolve this since I am now using `TxValidationState`

📝 22388o opened a pull request: "doc: upgrade Bitcoin Core license to 2024"
(https://github.com/bitcoin/bitcoin/pull/29142)
See [239450](https://github.com/bitcoin/bitcoin/pull/239450), [20805] (https://github.com/bitcoin/bitcoin/pull/20805), [17801](https://github.com/bitcoin/bitcoin/pull/17801), [15061](https://github.com/bitcoin/bitcoin/pull/15061) , [26748](https://github.com/bitcoin/bitcoin/pull/26748)

Every year had his upgrade :)

🤔 maflcko requested changes to a pull request: "doc: upgrade Bitcoin Core license to 2024"
(https://github.com/bitcoin/bitcoin/pull/29142#pullrequestreview-1795889686)
It should be enough to just link to last years' change in the description. Also, make sure the changes are complete and the commit message is accurate.

💬 maflcko commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868987998)
The error seems to be: ` node0 stderr bitcoin-node: wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion `it.second' failed. `?

So I think another bug would be that the error is not caught and reported by the python code?

💬 22388o commented on pull request "doc: upgrade Bitcoin Core license to 2024":
(https://github.com/bitcoin/bitcoin/pull/29142#issuecomment-1868989208)
> It should be enough to just link to last years' change in the description. Also, make sure the changes are complete and the commit message is accurate.

Ok

💬 22388o commented on pull request "doc: upgrade Bitcoin Core license to 2024":
(https://github.com/bitcoin/bitcoin/pull/29142#issuecomment-1868989343)
> It should be enough to just link to last years' change in the description. Also, make sure the changes are complete and the commit message is accurate.

So I can close PR?

💬 maflcko commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1868992264)
I guess this is fine, because the shift is constant and the compiler can skip the expensive modulo?

https://github.com/llvm/llvm-project/blob/9fba1d5f3a52af0ae62f386d0c494bd9510fa845/libcxx/include/__bit/rotate.h#L35

💬 maflcko commented on pull request "doc: upgrade Bitcoin Core license to 2024":
(https://github.com/bitcoin/bitcoin/pull/29142#issuecomment-1868992681)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 dimitaracev commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868993395)
Maybe we should return a proper message that indicates that the wallet we are trying to unload is already marked for unloading and we can catch that in the python code.

💬 sipa commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1869001285)
@maflcko Constant propagation + strength reduction should make that code not emit an actual modulo operation in any compiler from the last 20 years.

But hopefully that pattern in libc++ also matches whatever pattern matching clang has to make sure it instead emits an actual rot instruction on platforms where it exists.

💬 Lajandra commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1869042800)
> https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515 :
>
> ```shell
> _20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
> node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
> node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/
...

💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869080735)
Ok I don't agree that that would be an improvement, because:

1. The path already contains the word "example" (`examples/bitcoin.conf`)
2. Changing that file's name would also require an additional comment in the header instructing the user to rename the file as well as copy it to their data directory
3. The default datadir for all platforms is very clear in several places in the docs, and that default datadir is never *inside* the code repository