Bitcoin Core Github
42 subscribers
125K links
Download Telegram
πŸ‘ ismaelsadeeq approved a pull request: "test: fix `TestShell` initialization (late follow-up for #30463)"
(https://github.com/bitcoin/bitcoin/pull/30714#pullrequestreview-2261197556)
I verified that this behavior occurs locally:

```python
Python 3.12.4 (main, Jun 6 2024, 18:26:44) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/Users/abubakarismail/Desktop/bitcoin-dev/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/U
...
πŸ’¬ Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2310705314)
Alright then, dropping `g_best_block` as well!

Some tests failed with `EnsureAnyNodeContext(context).notifications`, so I fixed that (relative to the above patches). In `init.cpp` the RPC server starts at step 4a, but the notifications are added at step 7. This case is hit by several functional tests that shut the node down early.
πŸ“ Prabhat1308 opened a pull request: "rpc: Add test-only RPCs under `-test=<option>` flag"
(https://github.com/bitcoin/bitcoin/pull/30717)
followup of #29007 , implements adding of test-only RPC options in `-test` as discussed [here](https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950)

Currently a general user has access to `test-only` RPCs , where user might accidentally use it in production. This PR splits off test-only RPCs to be included in `-test` flag with additional restrictions of use in `regtest` segregating the user facing and testing RPCs . These test-only RPCs are split off from args and have their o
...
πŸ€” hebasto reviewed a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2261245718)
I still see IWYU warnings in https://cirrus-ci.com/task/5307830952001536. Here are some of them:

```
bench/addrman.cpp should add these lines:
#include "netaddress.h" // for CService, CNetAddr, Network
#include "protocol.h" // for CAddress, ServiceFlags
```
```
bench/bech32.cpp should remove these lines:
- #include <cstdint> // lines 9-9
```
```
bench/bench.h should add these lines:
#include "nanobench.h" // for Bench (ptr only)
```
```
bench/data.cpp should add th
...
πŸ’¬ murchandamus commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731615240)
From what I understand, the startup options `deprecatedrpc`, `stopatheight`, `limitancestorcount`, `limitancestorsize`, `limitdescendantcount`, and `limitdescendantsize` are definitely used in production by some users.
πŸ€” murchandamus reviewed a pull request: "rpc: Add test-only RPCs under `-test=<option>` flag"
(https://github.com/bitcoin/bitcoin/pull/30717#pullrequestreview-2261289148)
Looking at the discussion in #29007, I had the impression that it was suggested to move some RPC options to a "test-only" grouping, so I’m a bit surprised that this PR also moves a number of startup options to "test-only".

I’m wondering whether there was a misunderstanding regarding "RPC options" and "startup options" here.
πŸ’¬ martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2310776625)
@ismaelsadeeq rebased and pushed. thanks for the review!
πŸ“ theStack opened a pull request: "test: switch MiniWallet padding unit from weight to vsize "
(https://github.com/bitcoin/bitcoin/pull/30718)
This PR is a late follow-up for #30162, where I retrospectively consider the padding unit of choice as a mistake. The weight unit is merely a consensus rule detail and is largely irrelevant from a user's perspective w.r.t. fee-rate calculations and mempool policy rules (e.g. for package relay and TRUC limits), so there doesn't seem to be any value of using a granularity that we can't even guarantee to reach exactly anyway.

Switch to the more natural unit of vsize instead, which simplifies bot
...
πŸ’¬ Prabhat1308 commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731625165)
In the arguments description in `init.cpp` they are categorised as `DEBUG_ONLY` AND `DEBUG_TEST` which is where I picked them up to be "test-only". There was not much documentation elsewhere to decide otherwise.
πŸ’¬ murchandamus commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731628845)
I see. What’s the overarching intent here: is the purpose just to label them more explicitly as being for testing, or is the intent to restrict their use to test networks?
πŸ‘ jaonoctus approved a pull request: "seeds: Pull additional nodes from my seeder and update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/30008#pullrequestreview-2261317336)
utACK
πŸ’¬ Prabhat1308 commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731633356)
We are restricting its use in test networks only specifically to `regtest` only . the `-test` flag has a check to assert use of `regtest`.
πŸ’¬ achow101 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#issuecomment-2310802609)
ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b
πŸ’¬ achow101 commented on pull request "test: Add time-timewarp-attack boundary cases":
(https://github.com/bitcoin/bitcoin/pull/30698#issuecomment-2310813586)
ACK 31378d44f44cabc576bf92ddd61afde4b528de77

Going to merge this now in the interest of having test coverage of the boundary before the 28.x branch off.
πŸš€ achow101 merged a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669)
πŸ€” hodlinator requested changes to a pull request: "test: Improve clarity of subsidy limit test"
(https://github.com/bitcoin/bitcoin/pull/30699#pullrequestreview-2261348316)
Think a possible way forward would be to undo most changes to `subsidy_limit_test`.
* The change from `2099999997690000` to using `'`-separators is nice. Have you considered the slightly more BTC-native `20'999'999'9769'0000` or `20'999'999'97'690'000`? (Only found some vague precedence for the former in `util_ParseMoney` test case and none for the latter).
* As I already [mentioned](https://github.com/bitcoin/bitcoin/pull/30699#pullrequestreview-2255459494), I think it would be an improvement
...
πŸš€ achow101 merged a pull request: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698)
πŸ’¬ murchandamus commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731663858)
Sorry, I have to withdraw `limitancestorcount`, `limitancestorsize`, `limitdescendantcount`, and `limitdescendantsize`. Both parties that I suspected to use these startup options, in fact, do not use them. We definitely used `stopatheight` at a prior employer while indexing the blockchain, and my understanding was that `deprecatedrpc` was meant to give users a heads-up that an RPC would be going away with the next release, but not completely requiring an immediate transitioning away from it.
πŸ’¬ achow101 commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2310853293)
ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c
πŸ’¬ achow101 commented on pull request "devtools, utxo-snapshot: Fix block height out of range in script":
(https://github.com/bitcoin/bitcoin/pull/30690#issuecomment-2310859905)
ACK 5b4f34006dbd76223b55b156421f87d2241ac296