💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627554476)
nit in 0b92cd2ae4a317be4fe0dfd208017a7d05d1eb2a:
Looks like this commit is doing several things at once. It may be easier to review if move-only stuff (like moving rand256 and randbytes function bodies to the header file, and removing the explicit template instantiation) was done in a separate prepare commit?
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627554476)
nit in 0b92cd2ae4a317be4fe0dfd208017a7d05d1eb2a:
Looks like this commit is doing several things at once. It may be easier to review if move-only stuff (like moving rand256 and randbytes function bodies to the header file, and removing the explicit template instantiation) was done in a separate prepare commit?
💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1627806235)
perfect thanks
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1627806235)
perfect thanks
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627807244)
nit in c099cd2e3c20fc416e3f7fc1da4798bb8042af1c: Could increase the severity to `LogError()` while touching this?
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627807244)
nit in c099cd2e3c20fc416e3f7fc1da4798bb8042af1c: Could increase the severity to `LogError()` while touching this?
📝 marcofleon opened a pull request: "fuzz: add I2P harness"
(https://github.com/bitcoin/bitcoin/pull/30230)
Addresses https://github.com/bitcoin/bitcoin/issues/28803.
The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.
<details>
<summary>I2P dict</summary>
```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SE
...
(https://github.com/bitcoin/bitcoin/pull/30230)
Addresses https://github.com/bitcoin/bitcoin/issues/28803.
The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.
<details>
<summary>I2P dict</summary>
```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SE
...
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627821690)
Well in my view, concepts are really a kind of compiler-enforced documentation. Their primary purpose is giving a readable compiler error when template instantiation ought to be (and often, would be) failing anyway. In this case, someone implementing a new RNG but say missing `rand64`, would instead of getting an error in the instantiation of `RandomMixin` get an error message that references `RandomNumberGenerator`, which then explicitly lists what one is required to do to make it work.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627821690)
Well in my view, concepts are really a kind of compiler-enforced documentation. Their primary purpose is giving a readable compiler error when template instantiation ought to be (and often, would be) failing anyway. In this case, someone implementing a new RNG but say missing `rand64`, would instead of getting an error in the instantiation of `RandomMixin` get an error message that references `RandomNumberGenerator`, which then explicitly lists what one is required to do to make it work.
🤔 furszy reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2099269310)
ACK 9db2d8b
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2099269310)
ACK 9db2d8b
💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1627825496)
> I've made `m_batch` public so that we can just pass the `WalletBatch` but still use the underlying batch object. However I'm not sure if that's something we want to do in the long term, but I think it should be okay.
Yeah.. I'm not happy reading all the the `batch.m_batch` usages neither but np.
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1627825496)
> I've made `m_batch` public so that we can just pass the `WalletBatch` but still use the underlying batch object. However I'm not sure if that's something we want to do in the long term, but I think it should be okay.
Yeah.. I'm not happy reading all the the `batch.m_batch` usages neither but np.
📝 fanquake opened a pull request: "guix: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126"
(https://github.com/bitcoin/bitcoin/pull/30231)
Includes:
LLVM 18.1.x (#30201)
GCC 13.x (#29881)
git-minimal 2.41.0 -> 2.45.1
Kernel Headers 6.1.80 -> 6.1.92
moreutils 0.68 -> 0.69
Commits like https://git.savannah.gnu.org/cgit/guix.git/commit/?id=7b0f145802f0c2c785014293d748721678fef824, which should improve the bootstrap situation (#30042). This can somewhat be visualised by comparing the (simplified) dependencies of guix itself, between the two time-machines.
Master:
Includes:
LLVM 18.1.x (#30201)
GCC 13.x (#29881)
git-minimal 2.41.0 -> 2.45.1
Kernel Headers 6.1.80 -> 6.1.92
moreutils 0.68 -> 0.69
Commits like https://git.savannah.gnu.org/cgit/guix.git/commit/?id=7b0f145802f0c2c785014293d748721678fef824, which should improve the bootstrap situation (#30042). This can somewhat be visualised by comparing the (simplified) dependencies of guix itself, between the two time-machines.
Master:
![master_2](https://github.com/bitcoin/bitcoin/assets/86373
...
👍 theuni approved a pull request: "build: no-longer allow GCC-10 in C++20 check"
(https://github.com/bitcoin/bitcoin/pull/30228#pullrequestreview-2099370106)
utACK 232928b58a82e3f15307deba1ae921ae2960ccc8
(https://github.com/bitcoin/bitcoin/pull/30228#pullrequestreview-2099370106)
utACK 232928b58a82e3f15307deba1ae921ae2960ccc8
📝 luke-jr opened a pull request: "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options"
(https://github.com/bitcoin/bitcoin/pull/30232)
(https://github.com/bitcoin/bitcoin/pull/30232)
💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150152980)
@kristapsk @jonatack @achow101
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150152980)
@kristapsk @jonatack @achow101
💬 theStack commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-2150153924)
Rebased on master.
(https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-2150153924)
Rebased on master.
💬 hebasto commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150158568)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214.
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150158568)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214.
💬 hebasto commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2150163721)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214 and https://github.com/hebasto/bitcoin/pull/220.
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2150163721)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214 and https://github.com/hebasto/bitcoin/pull/220.
💬 theStack commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1627897054)
Such a test case was already introduced in the recently merged PR #28307 (commit 9be6065cc03f2408f290a332b203eef9c9cebf24). Note that this PR is primarily focused on the `keys_to_multisig_script` helper in the test framework and only uses the `createmultisig` RPC to verify it.
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1627897054)
Such a test case was already introduced in the recently merged PR #28307 (commit 9be6065cc03f2408f290a332b203eef9c9cebf24). Note that this PR is primarily focused on the `keys_to_multisig_script` helper in the test framework and only uses the `createmultisig` RPC to verify it.
💬 hebasto commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2150170440)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214 and https://github.com/hebasto/bitcoin/pull/220.
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2150170440)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214 and https://github.com/hebasto/bitcoin/pull/220.
💬 theStack commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1627904057)
Good question. I'm not sure if it's really that common, but there was a wish to also test the utility function via a functional test (see comment https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-1690367321) and it seemed to be a worthwhile idea to me.
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1627904057)
Good question. I'm not sure if it's really that common, but there was a wish to also test the utility function via a functional test (see comment https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-1690367321) and it seemed to be a worthwhile idea to me.
💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150205568)
@hebasto I think this is only ~half of what needs to be done here. Need to port #30137 as well, which should make this even simpler for CMake.
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150205568)
@hebasto I think this is only ~half of what needs to be done here. Need to port #30137 as well, which should make this even simpler for CMake.
💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150209122)
Ah nm, I see, that's already done :)
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2150209122)
Ah nm, I see, that's already done :)
💬 alfonsoromanz commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2150252609)
> Why not just expanding the tests in `wallet_basic.py`?
I initially added the test to `rpc_blockchain.py` because it focuses on testing RPC commands from `src/rpc/blockchain.cpp`. However, I'm happy to expand the tests in `wallet_basic.py` if that's preferred.
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2150252609)
> Why not just expanding the tests in `wallet_basic.py`?
I initially added the test to `rpc_blockchain.py` because it focuses on testing RPC commands from `src/rpc/blockchain.cpp`. However, I'm happy to expand the tests in `wallet_basic.py` if that's preferred.