💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729138611)
> makes the definitions inconsistent and adds noise
I agree that it's likely just a documentation change for the reader (unsurprisingly, my trivial godbolt experiment also showed the same compiled output), but the methods *are* a bit different, isn't it useful signal if we mark them as such?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729138611)
> makes the definitions inconsistent and adds noise
I agree that it's likely just a documentation change for the reader (unsurprisingly, my trivial godbolt experiment also showed the same compiled output), but the methods *are* a bit different, isn't it useful signal if we mark them as such?
💬 fanquake commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2307312714)
~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2307312714)
~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.
👍 TheCharlatan approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2257534072)
ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8
This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2257534072)
ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8
This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .
⚠️ maflcko opened an issue: "Intermittent timeout on p2p_ibd_stalling.py"
(https://github.com/bitcoin/bitcoin/issues/30704)
On slow hardware (e.g. qemu s390x), the test may take long enough for a block timeout to hit.
Should be easy to fix with by using mocktime.
Test folder: https://drahtbot.space/temp_scratch/p2p_ibd_stalling_23.tar.zstd
Log:
```
node0 2024-08-22T06:47:16.815656Z [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 18f9c9ae8651e9aeb776ec51943c7fb4b813b1dedae262459d1915f69c345f52 from peer=0, disconnecting
node0 2024-08-22T06:47:16.815964Z [msghand] [net_proc
...
(https://github.com/bitcoin/bitcoin/issues/30704)
On slow hardware (e.g. qemu s390x), the test may take long enough for a block timeout to hit.
Should be easy to fix with by using mocktime.
Test folder: https://drahtbot.space/temp_scratch/p2p_ibd_stalling_23.tar.zstd
Log:
```
node0 2024-08-22T06:47:16.815656Z [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 18f9c9ae8651e9aeb776ec51943c7fb4b813b1dedae262459d1915f69c345f52 from peer=0, disconnecting
node0 2024-08-22T06:47:16.815964Z [msghand] [net_proc
...
📝 maflcko opened a pull request: "test: Avoid intermittent block download timeout in p2p_ibd_stalling"
(https://github.com/bitcoin/bitcoin/pull/30705)
Fixes #30704
The goal of the test is to check the stalling timeout, not the block download timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 1023 blocks may take longer than the block download timeout.
Fix it by pinning the time using mocktime, and only advance it when testing the stalling timeout.
(https://github.com/bitcoin/bitcoin/pull/30705)
Fixes #30704
The goal of the test is to check the stalling timeout, not the block download timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 1023 blocks may take longer than the block download timeout.
Fix it by pinning the time using mocktime, and only advance it when testing the stalling timeout.
💬 pablomartin4btc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1729191860)
Building with `depends` on macOS setting the host as `arm64-apple-darwin` (following the notes in the readme file) I got confused noticing that the cross compilation was activated in the configuration (which was solved originally in https://github.com/hebasto/bitcoin/pull/125), then I checked in Ubuntu 22.04 which works fine and rerun the command on macOS without specifying `HOST=` and I found that in M3 the actual triplet is `aarch64-apple-darwin`, should we add it? Perhaps in a follow-up).
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1729191860)
Building with `depends` on macOS setting the host as `arm64-apple-darwin` (following the notes in the readme file) I got confused noticing that the cross compilation was activated in the configuration (which was solved originally in https://github.com/hebasto/bitcoin/pull/125), then I checked in Ubuntu 22.04 which works fine and rerun the command on macOS without specifying `HOST=` and I found that in M3 the actual triplet is `aarch64-apple-darwin`, should we add it? Perhaps in a follow-up).
🤔 pablomartin4btc reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2257577490)
re-ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Built successfully on macOS 14.4 with/ without `GUI` and legacy wallet. Noticed I didn't have `ccache` installed so I also played with/ without it and reviewed the `productivity.md` doc.
Also ran the tests with `ctest` and performed the `deploy` that had a in issue when compiling with `--target` identified by @fanquake [earlier](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288437679) and I've verified that works perfect now.
...
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2257577490)
re-ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Built successfully on macOS 14.4 with/ without `GUI` and legacy wallet. Noticed I didn't have `ccache` installed so I also played with/ without it and reviewed the `productivity.md` doc.
Also ran the tests with `ctest` and performed the `deploy` that had a in issue when compiling with `--target` identified by @fanquake [earlier](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288437679) and I've verified that works perfect now.
...
🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2257590987)
Reviewed up to e164614e32010bc8061cc99ed486188235efa780 (i.e. everything modulo fuzz and unit tests in the last 3 commits), lgtm so far.
Slightly off-topic, but: on some commits like 4e58d84da95d97d64f563b5c3e116769cd9ff89b `--color-moved=dimmed-zebra` didn't detect any lines as move-only for me (with 2-3 exceptions that seem completely arbitrary), any suggestions for advanced git options and/or recommendations for other fancy diff tools that can cope with that? (Or well, maybe I should just
...
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2257590987)
Reviewed up to e164614e32010bc8061cc99ed486188235efa780 (i.e. everything modulo fuzz and unit tests in the last 3 commits), lgtm so far.
Slightly off-topic, but: on some commits like 4e58d84da95d97d64f563b5c3e116769cd9ff89b `--color-moved=dimmed-zebra` didn't detect any lines as move-only for me (with 2-3 exceptions that seem completely arbitrary), any suggestions for advanced git options and/or recommendations for other fancy diff tools that can cope with that? (Or well, maybe I should just
...
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729201357)
just to be sure, was it intentional to use the bit-wise AND operator here, rather than the logical one (`&&=`), to avoid potential short-circuiting (see e.g. https://stackoverflow.com/questions/23107162/do-the-and-operators-for-bool-short-circuit)? IIUC, `add_extra_compact_tx` is always true at this point anyway (otherwise the if condition at the very top couldn't be true), so could also just do a regular assignment, I guess. But not sure what's better, one could argue that the current way is mo
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729201357)
just to be sure, was it intentional to use the bit-wise AND operator here, rather than the logical one (`&&=`), to avoid potential short-circuiting (see e.g. https://stackoverflow.com/questions/23107162/do-the-and-operators-for-bool-short-circuit)? IIUC, `add_extra_compact_tx` is always true at this point anyway (otherwise the if condition at the very top couldn't be true), so could also just do a regular assignment, I guess. But not sure what's better, one could argue that the current way is mo
...
💬 sancoder commented on issue "-proxy does not work for addresses like 10.x.y.z":
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2307425897)
> Bitcoin nodes keep a list of possible addresses of other nodes to connect to. This list is maintained by gossiping known addresses between nodes, including advertising own address to the other nodes, so that others know our address and can connect to us. From this perspective, it is useless to advertise an address like `10.x.y.z` to others on the internet because they will not be able to connect back. This is why we have a special `NET_UNROUTABLE` in the code.
Thank you for explaining this.
...
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2307425897)
> Bitcoin nodes keep a list of possible addresses of other nodes to connect to. This list is maintained by gossiping known addresses between nodes, including advertising own address to the other nodes, so that others know our address and can connect to us. From this perspective, it is useless to advertise an address like `10.x.y.z` to others on the internet because they will not be able to connect back. This is why we have a special `NET_UNROUTABLE` in the code.
Thank you for explaining this.
...
🤔 theStack reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2257664356)
lgtm, once again some smaller suggestions
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2257664356)
lgtm, once again some smaller suggestions
💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729249780)
nit: using the pre-generated chain is the default, so could just remove this line
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729249780)
nit: using the pre-generated chain is the default, so could just remove this line
💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729251710)
I assume this line can also be removed
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729251710)
I assume this line can also be removed
💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729250880)
could use the `vout` result from `send_to_address` also here, so the `find_vout_for_address` helper isn't needed anymore
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729250880)
could use the `vout` result from `send_to_address` also here, so the `find_vout_for_address` helper isn't needed anymore
💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287555)
Yup, this is more coherent. Updated.
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287555)
Yup, this is more coherent. Updated.
💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287638)
Updated
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287638)
Updated
💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307515830)
@theStack thank you for the suggestions! just forcePushed a new commit with the changes.
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307515830)
@theStack thank you for the suggestions! just forcePushed a new commit with the changes.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729318481)
Since the `consteval`/`constexpr`/`inline` choices are not obvious, but have fairly solid reasoning behind them, I could add non-Doxygen comments like
```C++
// Calls non-constexpr function so can only be inline
inline auto operator""_hex_v_u8() { return std::vector<uint8_t>{UCharCast(...
``
What do you think?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729318481)
Since the `consteval`/`constexpr`/`inline` choices are not obvious, but have fairly solid reasoning behind them, I could add non-Doxygen comments like
```C++
// Calls non-constexpr function so can only be inline
inline auto operator""_hex_v_u8() { return std::vector<uint8_t>{UCharCast(...
``
What do you think?
💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729321677)
That wouldn't explain to me why the compiler seems to allow it
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729321677)
That wouldn't explain to me why the compiler seems to allow it
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729323391)
Interesting idea!
Should move it out of the `detail` namespace. That makes the naming of `Hex` more important. Are people still happy with it?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729323391)
Interesting idea!
Should move it out of the `detail` namespace. That makes the naming of `Hex` more important. Are people still happy with it?