💬 naumenkogs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1126306044)
nevermind.
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1126306044)
nevermind.
💬 hebasto commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757)
This PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.
For example, for `riscv64-linux-gnu`
```
$ make -C src bitcoind
...
CXXLD bitcoind
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743':
net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsig
...
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757)
This PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.
For example, for `riscv64-linux-gnu`
```
$ make -C src bitcoind
...
CXXLD bitcoind
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743':
net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsig
...
💬 hebasto commented on pull request "util: add missing include and fix function signature":
(https://github.com/bitcoin/bitcoin/pull/27192#issuecomment-1456026433)
Post-merge ACK.
(https://github.com/bitcoin/bitcoin/pull/27192#issuecomment-1456026433)
Post-merge ACK.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126270559)
> Is m_ready_to_calculate an error indication, since it seems it can be false only If we hit the DoS limit?
No it's not just for indicating DoS limit reached, notice that it is also set to false after `BuildMockTemplate` is called. It prevents somebody from constructing a `MiniMiner` and calling `CalculateBumpFees()` or `CalculateTotalBumpFees()` multiple times, which could result in very incorrect results if the target feerate changes.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126270559)
> Is m_ready_to_calculate an error indication, since it seems it can be false only If we hit the DoS limit?
No it's not just for indicating DoS limit reached, notice that it is also set to false after `BuildMockTemplate` is called. It prevents somebody from constructing a `MiniMiner` and calling `CalculateBumpFees()` or `CalculateTotalBumpFees()` multiple times, which could result in very incorrect results if the target feerate changes.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126320439)
nit: It seems the rename to `GatherClusters` should have been squashed into efeee8b51d, but is in 5ee6f89e40 instead?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126320439)
nit: It seems the rename to `GatherClusters` should have been squashed into efeee8b51d, but is in 5ee6f89e40 instead?
📝 MarcoFalke opened a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
Remove long unused travis leftover
(https://github.com/bitcoin/bitcoin/pull/27209)
Remove long unused travis leftover
👍 fanquake approved a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
💬 willcl-ark commented on pull request "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk":
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456083159)
Concept ACK for moving to monotonic clocks.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is _additional_ hardening for a user whose system clock happens to be changed _while_ one of these is executing?
I couldn't easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to `GetRandHash()` or `GetRandBytes()
...
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456083159)
Concept ACK for moving to monotonic clocks.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is _additional_ hardening for a user whose system clock happens to be changed _while_ one of these is executing?
I couldn't easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to `GetRandHash()` or `GetRandBytes()
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384700)
well, this method is just a push function. It doesn't have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn't need to explain how APS works.
If you are strong over the bool args usage, that would be the argument to make the changes for me.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384700)
well, this method is just a push function. It doesn't have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn't need to explain how APS works.
If you are strong over the bool args usage, that would be the argument to make the changes for me.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384935)
sounds good
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384935)
sounds good
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126387031)
pushed, thanks
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126387031)
pushed, thanks
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456099290)
Updated per feedback, thanks S3RK 👌🏼.
[Small diff](https://github.com/bitcoin/bitcoin/compare/1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4..6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44), changes are in the last test scenario and the test commit message.
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456099290)
Updated per feedback, thanks S3RK 👌🏼.
[Small diff](https://github.com/bitcoin/bitcoin/compare/1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4..6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44), changes are in the last test scenario and the test commit message.
💬 MarcoFalke commented on pull request "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk":
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456125811)
> Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
yes, I don't think the system clock is ever adjusted in tests. Usually, we'd use `NodeClock` for a mockable clock. `faketime` may be a way to mock the system clock in tests, but faketime may not be available on Windows.
About the use
...
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456125811)
> Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
yes, I don't think the system clock is ever adjusted in tests. Usually, we'd use `NodeClock` for a mockable clock. `faketime` may be a way to mock the system clock in tests, but faketime may not be available on Windows.
About the use
...
📝 vasild opened a pull request: "gui: use the stored CSubNet entry when unbanning"
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1456152963)
@mzumsande, it is possible to avoid calling `LookupSubNet()` from the GUI, see https://github.com/bitcoin-core/gui/pull/717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1456152963)
@mzumsande, it is possible to avoid calling `LookupSubNet()` from the GUI, see https://github.com/bitcoin-core/gui/pull/717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.
💬 vasild commented on pull request "gui: use the stored CSubNet entry when unbanning":
(https://github.com/bitcoin-core/gui/pull/717#issuecomment-1456154849)
This would simplify https://github.com/bitcoin/bitcoin/pull/27071.
(https://github.com/bitcoin-core/gui/pull/717#issuecomment-1456154849)
This would simplify https://github.com/bitcoin/bitcoin/pull/27071.
💬 petertodd commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1456161328)
So the next step here for people who want to actually see this implemented is code, and perhaps more importantly, tests.
The code change is fairly simple: change the `datacarrierlimit` to default to max-int, and make the rule that checks if there is more than one OP_Return only take action if the limit isn't the default. I don't think there is any reason to implement a separate rule to check if there is more than one OP_Return if you aren't limiting the size.
Second, tests. You need to che
...
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1456161328)
So the next step here for people who want to actually see this implemented is code, and perhaps more importantly, tests.
The code change is fairly simple: change the `datacarrierlimit` to default to max-int, and make the rule that checks if there is more than one OP_Return only take action if the limit isn't the default. I don't think there is any reason to implement a separate rule to check if there is more than one OP_Return if you aren't limiting the size.
Second, tests. You need to che
...
👍 dergoegge approved a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
🚀 glozow merged a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
(https://github.com/bitcoin/bitcoin/pull/27209)
💬 MarcoFalke commented on pull request "ci: Cache more stuff in the ci images: msan, iwyu, pip, sdks":
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1456213291)
Rebased. For review I suggest `--color-moved=dimmed-zebra`.
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1456213291)
Rebased. For review I suggest `--color-moved=dimmed-zebra`.
💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456215491)
Yeah, the same happens on Lunar (gcc 12.2). Steps to reproduce on a fresh install:
```
export HOST=powerpc64le-linux-gnu && export MAKEJOBS="$(nproc)" && apt update && apt install git vim htop -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install -y bzip2 make automake cmake curl g++-multilib libtool binutils bsdmainutils pkg-config python3 patch bison && apt install -y g++-arm-linux-gnueabihf binutils-arm-linux-gnueabihf
...
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456215491)
Yeah, the same happens on Lunar (gcc 12.2). Steps to reproduce on a fresh install:
```
export HOST=powerpc64le-linux-gnu && export MAKEJOBS="$(nproc)" && apt update && apt install git vim htop -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install -y bzip2 make automake cmake curl g++-multilib libtool binutils bsdmainutils pkg-config python3 patch bison && apt install -y g++-arm-linux-gnueabihf binutils-arm-linux-gnueabihf
...