🤔 janb84 reviewed a pull request: "docs: clarify RPC credentials security boundary"
(https://github.com/bitcoin/bitcoin/pull/32424#pullrequestreview-2820704745)
reACK [348dc97](https://github.com/bitcoin/bitcoin/commit/348dc97608e0988e1974200cd531b29477dd374e)
Changes since last ACK:
- Small textual change
@crStiv thnx for incorporating my nit !
(https://github.com/bitcoin/bitcoin/pull/32424#pullrequestreview-2820704745)
reACK [348dc97](https://github.com/bitcoin/bitcoin/commit/348dc97608e0988e1974200cd531b29477dd374e)
Changes since last ACK:
- Small textual change
@crStiv thnx for incorporating my nit !
💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857488874)
Just beacuse these transactions are valid bitcoin transactions and pay fees is not a good argument against it being spam.
Spam in any protocol manages to make its way into the system by staying within the rules of the protocol. So all spam are valid protocol messages.
The reason why these data blobs are considered spam is because they are competing for block space against genuine financial transactions which is the usecase of bitcoin as money - not for storage of arbitrary data. Just beca
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857488874)
Just beacuse these transactions are valid bitcoin transactions and pay fees is not a good argument against it being spam.
Spam in any protocol manages to make its way into the system by staying within the rules of the protocol. So all spam are valid protocol messages.
The reason why these data blobs are considered spam is because they are competing for block space against genuine financial transactions which is the usecase of bitcoin as money - not for storage of arbitrary data. Just beca
...
💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857496855)
The reason why Bitcoin is the best monetary network IMHO is because we have maintained conservative values on what the chain is specialized to do.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857496855)
The reason why Bitcoin is the best monetary network IMHO is because we have maintained conservative values on what the chain is specialized to do.
📝 maflcko opened a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434)
The exclusion isn't needed. In fact, it prevents detection of `"bla" + wrong()`.
For example, the following is not detected:
```diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
RPCHelpMan keypoolrefill()
{
return RPCHelpMan{"keypoolrefill",
- "\nFills the keypool."+
+
...
(https://github.com/bitcoin/bitcoin/pull/32434)
The exclusion isn't needed. In fact, it prevents detection of `"bla" + wrong()`.
For example, the following is not detected:
```diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
RPCHelpMan keypoolrefill()
{
return RPCHelpMan{"keypoolrefill",
- "\nFills the keypool."+
+
...
💬 maflcko commented on pull request "docs: Improve `keypoolrefill` RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077012046)
```suggestion
"Refills each descriptor keypool in the wallet up to the specified number of new keys.\n"
```
nit: This is trimmed anyway.
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077012046)
```suggestion
"Refills each descriptor keypool in the wallet up to the specified number of new keys.\n"
```
nit: This is trimmed anyway.
💬 maflcko commented on pull request "docs: Improve `keypoolrefill` RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077039662)
I don't think to_string is allowed, see https://github.com/bitcoin/bitcoin/pull/32434
Could use ToString?
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077039662)
I don't think to_string is allowed, see https://github.com/bitcoin/bitcoin/pull/32434
Could use ToString?
💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2857505743)
GUIX build:
```
db8daad79781d10e52b1b225343fd72081ba717b484b44068ad10d646bdcb89c guix-build-345944bd6cf8/output/dist-archive/bitcoin-345944bd6cf8.tar.gz
55ceb9d4bd2f4707b6823d69403c659a8b1af96a24b0e4b2a6ad4a288a98214f guix-build-345944bd6cf8/output/x86_64-w64-mingw32/SHA256SUMS.part
978a9f8d8726d0fe1b81fa498f8601f7b33f88ac1fb9a4b71026edadd879d7b7 guix-build-345944bd6cf8/output/x86_64-w64-mingw32/bitcoin-345944bd6cf8-win64-codesigning.tar.gz
2668cceae2d6793ff38b25a42987b9c3b491c270377e3df
...
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2857505743)
GUIX build:
```
db8daad79781d10e52b1b225343fd72081ba717b484b44068ad10d646bdcb89c guix-build-345944bd6cf8/output/dist-archive/bitcoin-345944bd6cf8.tar.gz
55ceb9d4bd2f4707b6823d69403c659a8b1af96a24b0e4b2a6ad4a288a98214f guix-build-345944bd6cf8/output/x86_64-w64-mingw32/SHA256SUMS.part
978a9f8d8726d0fe1b81fa498f8601f7b33f88ac1fb9a4b71026edadd879d7b7 guix-build-345944bd6cf8/output/x86_64-w64-mingw32/bitcoin-345944bd6cf8-win64-codesigning.tar.gz
2668cceae2d6793ff38b25a42987b9c3b491c270377e3df
...
💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2857513292)
> Add usage notes for contrib/devtools/security-check.py
I think you can actually remove these docs. The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
> Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
This see
...
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2857513292)
> Add usage notes for contrib/devtools/security-check.py
I think you can actually remove these docs. The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
> Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
This see
...
💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857513687)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:
```
If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.
```
I hope the core devs to act with integrity and do what is
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857513687)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:
```
If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.
```
I hope the core devs to act with integrity and do what is
...
💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857514867)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:
**If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.**
I hope the core devs to act with integrity and do what is right
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857514867)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:
**If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.**
I hope the core devs to act with integrity and do what is right
...
💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2077060913)
Was this added to appease a linter? There should be no need for code like this, as every file passed to these scripts in Guix, is a binary. Leaving the script to fail if `.parse` fails is fine.
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2077060913)
Was this added to appease a linter? There should be no need for code like this, as every file passed to these scripts in Guix, is a binary. Leaving the script to fail if `.parse` fails is fine.
💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2857659697)
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08f
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2857659697)
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08f
💬 josibake commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2857741552)
Concept ACK
> A notable use-case for the kernel library is accessing and analyzing existing block data
A concrete example is index building in electrs / esplora / etc. For example, Electrs does this today by:
1. Waiting for Bitcoin Core to finish IBD
2. Reading all of the blocks out over JSON-RPC
3. Parsing them and writing them into the appropriate indexes
I started on a PoC for Electrs to use libbitcoinkernel for index building to demonstrate how this could be done much more effi
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2857741552)
Concept ACK
> A notable use-case for the kernel library is accessing and analyzing existing block data
A concrete example is index building in electrs / esplora / etc. For example, Electrs does this today by:
1. Waiting for Bitcoin Core to finish IBD
2. Reading all of the blocks out over JSON-RPC
3. Parsing them and writing them into the appropriate indexes
I started on a PoC for Electrs to use libbitcoinkernel for index building to demonstrate how this could be done much more effi
...
💬 willcl-ark commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2857758897)
I have just re-tested and do not find that removing `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH` sees the depends toolchain working correctly on a default NixOS install, using the repro steps in OP.
> I'd avoid making assumptions about undocumented CMAKE_PREFIX_PATH-related behaviour. Let's preserve the idempotence of the toolchain file.
Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:
1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`
2.
...
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2857758897)
I have just re-tested and do not find that removing `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH` sees the depends toolchain working correctly on a default NixOS install, using the repro steps in OP.
> I'd avoid making assumptions about undocumented CMAKE_PREFIX_PATH-related behaviour. Let's preserve the idempotence of the toolchain file.
Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:
1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`
2.
...
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2857767885)
Rebased after #32086.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2857767885)
Rebased after #32086.
💬 hebasto commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2857794327)
> Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:
>
> 1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`
Building our own depends implies that we fully rely on them, ideally skipping all other search paths used by CMake's package, library, and header search mechanisms. Therefore, I believe we should retain as many restrictions as possible.
> 2. Explicitly set `CMAKE_PREFIX_PATH` using an if-guard, as per: [437a4da](https://gith
...
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2857794327)
> Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:
>
> 1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`
Building our own depends implies that we fully rely on them, ideally skipping all other search paths used by CMake's package, library, and header search mechanisms. Therefore, I believe we should retain as many restrictions as possible.
> 2. Explicitly set `CMAKE_PREFIX_PATH` using an if-guard, as per: [437a4da](https://gith
...
💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2857833155)
> I'll compare the reindex-chainstate performance next, before & after, let's see if there's a regression (we do expect a 4% slowdown for max cache, like in IBD).
IBD seems to be basically the same as before, this change indeed only affects `reindex-chainstate`.
<details>
<summary>Details</summary>
```bash
COMMITS="baa848b8d38187ce6b24a57cfadf1ea180209711 c822dd1f355b6f403191072cdd3c9c3e234bbcaa"; \
STOP_HEIGHT=888888; DBCACHE=45000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage";
...
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2857833155)
> I'll compare the reindex-chainstate performance next, before & after, let's see if there's a regression (we do expect a 4% slowdown for max cache, like in IBD).
IBD seems to be basically the same as before, this change indeed only affects `reindex-chainstate`.
<details>
<summary>Details</summary>
```bash
COMMITS="baa848b8d38187ce6b24a57cfadf1ea180209711 c822dd1f355b6f403191072cdd3c9c3e234bbcaa"; \
STOP_HEIGHT=888888; DBCACHE=45000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage";
...
💬 sipsorcery commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2857899480)
tACK b074ae1b4f52471a9d71e9431deebae5ec3d603e native Windows build.
```
c:\dev\github\bitcoin>mt.exe -nologo -inputresource:build\bin\Release\bitcoind.exe -validate_manifest
Parsing of manifest successful.
```
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2857899480)
tACK b074ae1b4f52471a9d71e9431deebae5ec3d603e native Windows build.
```
c:\dev\github\bitcoin>mt.exe -nologo -inputresource:build\bin\Release\bitcoind.exe -validate_manifest
Parsing of manifest successful.
```
👍 l0rinc approved a pull request: "test: Add and use ElapseTime helper"
(https://github.com/bitcoin/bitcoin/pull/32430#pullrequestreview-2821125017)
Concept ACK - was thinking of something similar when reviewing https://github.com/bitcoin/bitcoin/pull/32414/commits/41479ed1d23ea752d0ce14c2cf5627f43bceb722#diff-0d33078447a7be03c4ca7c0c86e3f4b64f77afae719c7a353e86d688932bff3eR38
(https://github.com/bitcoin/bitcoin/pull/32430#pullrequestreview-2821125017)
Concept ACK - was thinking of something similar when reviewing https://github.com/bitcoin/bitcoin/pull/32414/commits/41479ed1d23ea752d0ce14c2cf5627f43bceb722#diff-0d33078447a7be03c4ca7c0c86e3f4b64f77afae719c7a353e86d688932bff3eR38
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-2857909160)
Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully :sweat_smile:
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-2857909160)
Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully :sweat_smile: