⚠️ mzumsande opened an issue: "Remove BIP94 from regtest"
(https://github.com/bitcoin/bitcoin/issues/31137)
[BIP94](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) which, amongst others, fixes the timewarp attack on testnet4, has been activated on regtest by #30681 (commit https://github.com/bitcoin/bitcoin/pull/30681/commits/e85f386c4b157b7d1ac16aface9bd2c614e62b46), in order to allow having a functional test for the new testnet4 behavior.
As I argued in https://github.com/bitcoin/bitcoin/pull/30681#pullrequestreview-2275751672 I believe that regtest's main task is to test mainnet
...
(https://github.com/bitcoin/bitcoin/issues/31137)
[BIP94](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) which, amongst others, fixes the timewarp attack on testnet4, has been activated on regtest by #30681 (commit https://github.com/bitcoin/bitcoin/pull/30681/commits/e85f386c4b157b7d1ac16aface9bd2c614e62b46), in order to allow having a functional test for the new testnet4 behavior.
As I argued in https://github.com/bitcoin/bitcoin/pull/30681#pullrequestreview-2275751672 I believe that regtest's main task is to test mainnet
...
💬 mzumsande commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2430181845)
Forgot about this until last week - I opened issue #31137 to continue discussion.
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2430181845)
Forgot about this until last week - I opened issue #31137 to continue discussion.
👋 jonatack's pull request is ready for review: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135)
(https://github.com/bitcoin/bitcoin/pull/31135)
👍 hodlinator approved a pull request: "fees: Remove CLIENT_VERSION serialization"
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2386255699)
ACK fa8bd0be8432fda3c7312050433a6deb6722a073
Have a slight preference for the commit removing CLIENT_VERSION and the commit introducing CURRENT_FEES_FILE_VERSION being one and the same in order to give a more complete picture of the change.
Used hexdump to inspect old *~/.bitcoin/regtest/fee_estimates.dat* and *~/.bitcoin/fee_estimates.dat* files, confirming that the first two little-endian integers correspond to the *minimum required version* and *CLIENT_VERSION* (version that wrote).
...
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2386255699)
ACK fa8bd0be8432fda3c7312050433a6deb6722a073
Have a slight preference for the commit removing CLIENT_VERSION and the commit introducing CURRENT_FEES_FILE_VERSION being one and the same in order to give a more complete picture of the change.
Used hexdump to inspect old *~/.bitcoin/regtest/fee_estimates.dat* and *~/.bitcoin/fee_estimates.dat* files, confirming that the first two little-endian integers correspond to the *minimum required version* and *CLIENT_VERSION* (version that wrote).
...
💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1811355454)
Yes, in case it somehow gets corrupted or modified by an entirely different software, that makes sense.
Appreciated if you re-touch.
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1811355454)
Yes, in case it somehow gets corrupted or modified by an entirely different software, that makes sense.
Appreciated if you re-touch.
💬 hodlinator commented on issue "Windows Python: Intermittent issue in p2p_unrequested_blocks.py", line 98, in run_test test_node.send_and_ping(msg_block(blocks_h2[0])) assert self.is_connected AssertionError / AcceptBlock FAILED (time-too-old, block's timestamp is too early) ":
(https://github.com/bitcoin/bitcoin/issues/29897#issuecomment-2430401806)
Encountered here: https://github.com/hodlinator/bitcoin/actions/runs/11460619753/job/31887736863#step:12:590
(Still running on a base of 84cd6478c422bc296589ab031f5c76e7bab2704d from 16 September though).
(https://github.com/bitcoin/bitcoin/issues/29897#issuecomment-2430401806)
Encountered here: https://github.com/hodlinator/bitcoin/actions/runs/11460619753/job/31887736863#step:12:590
(Still running on a base of 84cd6478c422bc296589ab031f5c76e7bab2704d from 16 September though).
💬 theStack commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2430509479)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2430509479)
Concept ACK
👍 tdb3 approved a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2380545052)
CR and test ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0
Checked with a hackie little sanity test where a connection was made to the proxy on 127.0.0.1:10000 (set up to detour to 127.0.0.1:10001). A small echo server listened on 10001 (echoing back "testack"). The new detouring functionality of the proxy seemed to work as expected.
<details>
<summary>hackie test</summary>
```diff
diff --git a/test/functional/feature_framework_unit_tests.py b/test/functional/feature_framework_unit_te
...
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2380545052)
CR and test ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0
Checked with a hackie little sanity test where a connection was made to the proxy on 127.0.0.1:10000 (set up to detour to 127.0.0.1:10001). A small echo server listened on 10001 (echoing back "testack"). The new detouring functionality of the proxy seemed to work as expected.
<details>
<summary>hackie test</summary>
```diff
diff --git a/test/functional/feature_framework_unit_tests.py b/test/functional/feature_framework_unit_te
...
💬 tdb3 commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1807873344)
non-blocking femto nit: If this file is touched again, could add a trailing comma to prevent the line from being changed if another import from netutil is needed.
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1807873344)
non-blocking femto nit: If this file is touched again, could add a trailing comma to prevent the line from being changed if another import from netutil is needed.
👍 rkrux approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2387132611)
tACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9
Successful make and functional tests. Pretty straightforward PR; I agree with @instagibbs's suggestion to keep the variable name in functional test same as in the CPP code.
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2387132611)
tACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9
Successful make and functional tests. Pretty straightforward PR; I agree with @instagibbs's suggestion to keep the variable name in functional test same as in the CPP code.
👍 rkrux approved a pull request: "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped"
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2387408321)
tACK bf46723537c37cb1ee7f84ffe7b75c253beadb89
Successful make and functional tests.
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2387408321)
tACK bf46723537c37cb1ee7f84ffe7b75c253beadb89
Successful make and functional tests.
💬 rkrux commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1811959750)
Let's keep the same name in the functional test for uniformity and easy reference?
https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.h#L27
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1811959750)
Let's keep the same name in the functional test for uniformity and easy reference?
https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.h#L27
💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072)
Worth bringing back the `try`/`catch` in *src/test/fuzz/strprintf.cpp* for Debug builds?
```
₿ cmake --preset=libfuzzer -DCMAKE_BUILD_TYPE=Debug
...
₿ cmake --build build_fuzz -j<X>
...
₿ FUZZ=str_printf build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 734467281
INFO: Loaded 1 modules (1292656 inline 8-bit counters): 1292656 [0x5d5503ee60a0, 0x5d5504021a10),
INFO: Loaded 1 PC tables (1292656 P
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072)
Worth bringing back the `try`/`catch` in *src/test/fuzz/strprintf.cpp* for Debug builds?
```
₿ cmake --preset=libfuzzer -DCMAKE_BUILD_TYPE=Debug
...
₿ cmake --build build_fuzz -j<X>
...
₿ FUZZ=str_printf build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 734467281
INFO: Loaded 1 modules (1292656 inline 8-bit counters): 1292656 [0x5d5503ee60a0, 0x5d5504021a10),
INFO: Loaded 1 PC tables (1292656 P
...
💬 fanquake commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2431330789)
@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.
> (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).
I would think the code would not be worse performance wise, as it's implemented using `__stosb`.
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2431330789)
@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.
> (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).
I would think the code would not be worse performance wise, as it's implemented using `__stosb`.
💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2431441971)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2431441971)
Rebased.
💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2431475513)
I am in favor of 2. (port+1) or 5. (do nothing) in the short term, for 28.1.
For long term, maybe deprecate and remove `port=` (yes, I think it is a subset of `bind=`, somebody correct me if I am wrong) in favor of `bind=...:port`. I agree that `port=` is "helpful, especially for users less knowledgeable about networking", maybe to resolve that extend the syntax of `bind=` to allow `bind=*:8333`. That is used in at least Apache configs: `<VirtualHost *:80>`.
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2431475513)
I am in favor of 2. (port+1) or 5. (do nothing) in the short term, for 28.1.
For long term, maybe deprecate and remove `port=` (yes, I think it is a subset of `bind=`, somebody correct me if I am wrong) in favor of `bind=...:port`. I agree that `port=` is "helpful, especially for users less knowledgeable about networking", maybe to resolve that extend the syntax of `bind=` to allow `bind=*:8333`. That is used in at least Apache configs: `<VirtualHost *:80>`.
💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2431486918)
Inputs for this are available at: https://github.com/brunoerg/qa-assets/pull/2
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2431486918)
Inputs for this are available at: https://github.com/brunoerg/qa-assets/pull/2
💬 brunoerg commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2431516183)
Thanks for it! I think this is similar to #30714 (which fixed the initialization).
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2431516183)
Thanks for it! I think this is similar to #30714 (which fixed the initialization).
💬 naiyoma commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2431610508)
Picking this up since all previous attempts are now closed.
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2431610508)
Picking this up since all previous attempts are now closed.
:lock: fanquake locked an issue: "COIN_VALUE no longer accessible in const contexts"
(https://github.com/bitcoin/bitcoin/issues/31113)
(https://github.com/bitcoin/bitcoin/issues/31113)