| Age | Commit message (Collapse) | Author |
|---|
|
|
|
Co-authored-by: Carl Dong <contact@carldong.me>
|
|
fee check
c4ddee64c7f80eee05a95116ef1b1dc8a7601183 test: Add test for replacement relay fee check (Antoine Riard)
Pull request description:
This PR adds rename the `reject_reason` of our implementation of BIP125 rule 4 and adds missing functional test coverage. Note, `insufficient fee` is already the `reject_reason` of few others `PreChecks` replacement checks and as such might be confusing.
> The replacement transaction must also pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting. For example, if the minimum relay fee is 1 satoshi/byte and the replacement transaction is 500 bytes total, then the replacement must pay a fee at least 500 satoshis higher than the sum of the originals.
```
// Finally in addition to paying more fees than the conflicts the
// new transaction must pay for its own bandwidth.
CAmount nDeltaFees = nModifiedFees - nConflictingFees;
if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
hash.ToString(),
FormatMoney(nDeltaFees),
FormatMoney(::incrementalRelayFee.GetFee(nSize))));
}
```
ACKs for top commit:
MarcoFalke:
cr ACK c4ddee64c7f80eee05a95116ef1b1dc8a7601183
glozow:
ACK c4ddee6, one small suggestion if you retouch.
Tree-SHA512: 7c5d1065db6e6fe57a9f083bf051a7a55eb9892de3a2888679d4a6853491608c93b6e35887ef383a9988d14713fa13a0b1d6134b7354af5fd54765f0d4e98568
|
|
3efaf83c75cd8dc2fa084537b8ed6715fb58c04d wallet: deactivate descriptor (S3RK)
6737d9655bcf527afbd85d610d805a2d0fd28c4f test: wallet importdescriptors update existing (S3RK)
586f1d53d60880ea2873d860f95e3390016620d1 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db14748d9ee04735b4968366d33bc89aea23 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd555f791103f81adc9111e0e55c8003 wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d
meshcollider:
Code review ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d
jonatack:
Light ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
|
|
|
|
use in normalized descriptors
e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886b60644d103a5eb310691c003a39328 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2f2e139fbc63e2f37fb33ea4efae088 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a83f3e94bcb78b6aa463c5570c1e737e Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8ba69e2cc82c617cdf406ab79df58825 wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e5434da90d2cf680f23e8c7b7164c9f945 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b834077f102724415e0fada6ee8b2def2 descriptors: Cache last hardened xpub (Andrew Chow)
cacc3910989c4f3d7afa530dbab042461426abce Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75cd03c8f0a8cfadb47e0fbcabe3c5e59 Refactor Cache merging and writing (Andrew Chow)
976b53b085d681645fd3a008fe382de85647e29f Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
|
|
|
|
|
|
|
|
when loading assumeutxo
fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
Pull request description:
It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.
Same for the outpoint index.
Both issues were found by fuzzing:
* The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
* The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
ACKs for top commit:
practicalswift:
cr ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4: patch looks correct
jamesob:
crACK https://github.com/bitcoin/bitcoin/pull/22146/commits/fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4
theStack:
Code review ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4
benthecarman:
crACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4
Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
|
|
With the last hardened xpub cache, we don't neeed to have the wallet be
unlocked for listdescriptors.
|
|
support for fetching bech32m addresses
754f134a50cc56cdf0baf996d909c992770fcc97 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7c0ffd545e537bd497cdc3e67d045f6 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d10728f882986ed0d9ed77bc736f051c662 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad9487a4e04c1ffc68211e84e126b3d2 Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34567743e527dad46912c9ba493252cd Add OutputType::BECH32M (Andrew Chow)
177c15d2f7cd5406ddbce8217fc023057539b828 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50cc56cdf0baf996d909c992770fcc97
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50cc56cdf0baf996d909c992770fcc97
jonatack:
ACK 754f134a50cc56cdf0baf996d909c992770fcc97
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
|
|
syncwithvalidationinterfacequeue in p2p_blockfilters
fadddd13eef4428f5fa7237583d4be41a9335cd9 test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e3d4984b8edff1d762dd4cba205d982 test: Misc cleanup (MarcoFalke)
fa1668bf5084a190b26b022b9e625a7be3defa6e test: Run pep-8 (MarcoFalke)
facd97ae0f0d816107aa3bc9de321244200636a0 scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK https://github.com/bitcoin/bitcoin/pull/22311/commits/fadddd13eef4428f5fa7237583d4be41a9335cd9 on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
|
|
helpers cleanups/improvements
bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
191405420815d49ab50184513717a303fc2744d6 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f8f81c78cf84117a87074c6ff6c9d95 test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958c4a10ba20dc86c011d71cda4b070a5 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
|
|
6168eb06b2044f00f18727b955b672fc91c60bd7 [test] Prevent intermittent issue (Amiti Uttarwar)
1d8193e2a2950fd957654b601e85ab888899c394 [test] Remove GetAddrStore class (Amiti Uttarwar)
ef2f149bf2d12e2d14e441fdf701808f0f1dfb8e [test] Update GetAddrStore callers to use AddrReceiver (Amiti Uttarwar)
e8c67ea19ac4c0aec4a0b449ac3a4152f80dfff5 [test] Add functionality to AddrReceiver (Amiti Uttarwar)
09dc073cff250afd47a3e219f35d1257add764e9 [test] Allow AddrReceiver to be used more generally (Amiti Uttarwar)
Pull request description:
A test refactor broken out from #21528 & a fix to #22243.
This PR:
1. consolidates the two helper classes into one, with the intent of making the test logic more clear & usable as we add more subtests to the file
2. hopefully fixes the test flakiness by bumping up the mocktime interval to ensure `m_next_addr_send` timer triggers
ACKs for top commit:
mzumsande:
Code-Review ACK 6168eb06b2044f00f18727b955b672fc91c60bd7
lsilva01:
Tested ACK https://github.com/bitcoin/bitcoin/pull/22306/commits/6168eb06b2044f00f18727b955b672fc91c60bd7 on Ubuntu 20.04
brunoerg:
tACK 6168eb06b2044f00f18727b955b672fc91c60bd7
Tree-SHA512: 248324f9d37e0e5ffe4acc437cd72ad9a2960abc868a97c6040a36e6ea8b59029127ac4f63fcf67d981a5bb4dbf2334bb2c23c541fae8e910d5523884bcedcba
|
|
fa4017e7a0899959b2ac84bcbc3f34dfb17b5fce refactor: Pass grind args vector as const reference (MarcoFalke)
fa08bc288f81dd42a482e2bfef37d21a1e5fddd2 Remove gArgs from AppInitUtil (MarcoFalke)
fa751a47ff4253f58518d7f43d33ec1227ea0dbc Remove unused OptionsCategory arg from AddCommand (MarcoFalke)
fa3c1eee7ff2646e78540d53b4f8eaf095a8c27d Remove unused includes from bitcoin-util (MarcoFalke)
fa304929e2c3583bc3e6b05eaa6e0df6cdac6ec8 test: Add bitcoin-util tests (MarcoFalke)
fa831e709a4d605a18e5de1627b48d670bb326fb build_msvc: Add bitcoin-util.exe (MarcoFalke)
Pull request description:
bitcoin-util has no tests
See https://marcofalke.github.io/btc_cov/total.coverage/src/bitcoin-util.cpp.gcov.html (Coverage report showing 0%)
ACKs for top commit:
klementtan:
Code review and tested ACK fa4017e7a0899959b2ac84bcbc3f34dfb17b5fce
theStack:
Tested ACK fa4017e7a0899959b2ac84bcbc3f34dfb17b5fce
jamesob:
reACK fa4017e7a0899959b2ac84bcbc3f34dfb17b5fce ([`jamesob/ackr/22270.1.MarcoFalke.test_add_bitcoin_util_te`](https://github.com/jamesob/bitcoin/tree/ackr/22270.1.MarcoFalke.test_add_bitcoin_util_te))
Tree-SHA512: 68e2791239bc48d28fbb6394155c39ea0357a96ec7e4896ca579feeef1a803657165a0ef9fa3cf6e2a381e5b0ca0dafa1b594158303a04997db784201d8dd66d
|
|
|
|
d637a9b397816e34652d0c4d383308e39770737a Taproot descriptor inference (Pieter Wuille)
c7388e5ada394b7fe94d6263fb02e9dd28ab367e Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b9a1879e6c3c7e153b2e6f33a79e905 consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397816e34652d0c4d383308e39770737a
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397816e34652d0c4d383308e39770737a
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
|
|
bb719a08db173a753984a04534de6f148b87b17a style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10ebda301548b8ff4b0c73fefc367ad5dc22b doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957dcdfc971a4a971995ff2db9fb787d23c3 test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977ae2076903ed12ab7616a7f93e88be02e1 banman: save the banlist in a JSON format on disk (Vasil Dimov)
Pull request description:
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
ACKs for top commit:
jonatack:
Code review re-ACK bb719a08db173a753984a04534de6f148b87b17a per `git range-diff 6a67366 4b52c72 bb719a0`
achow101:
Code Review ACK bb719a08db173a753984a04534de6f148b87b17a
Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
|
|
feature_coinstatsindex
fafd9165e911bf33d6212ca8a613b71878c82449 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)
Pull request description:
Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)
ACKs for top commit:
jamesob:
crACK https://github.com/bitcoin/bitcoin/pull/22313/commits/fafd9165e911bf33d6212ca8a613b71878c82449
Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
|
|
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
|
|
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
|
|
The tr() descriptor, WitnessV1Taproot CTxDestination, and
WitnessUnknown CTxDestination are OutputType::BECH32M so they should
report as such.
|
|
|
|
|
|
* Replace wait_until with assert_equal where possible
* Use send_and_ping helper where possible
|
|
Can be reviewed with --word-diff-regex=.
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" test/functional/p2p_blockfilters.py ; }
# Rename from "node" to "peer" to avoid confusion with self.nodes
ren node0 peer_0
ren node1 peer_1
# Remove the confusing "C" prefix
ren CFiltersClient FiltersClient
-END VERIFY SCRIPT-
|
|
This reverts commit faa94961d6e38392ba068381726ed4e033367b03.
|
|
Notebook
168b6c317ca054c1287c36be532964e861f44266 add dummy file param to fix jupyter (Josiah Baker)
Pull request description:
this fixes argparse to use `parse_known_args`. previously, if an unknown argument was passed, argparse would fail with an `unrecognized arguments: %s` error.
## why
the documentation mentions being able to run `TestShell` in a REPL interpreter or a jupyter notebook. when i tried to run inside a jupyter notebook, i got the following error:
this was due to the notebook passing the filename of the notebook as an argument. this is a known problem with notebooks and argparse, documented here: https://stackoverflow.com/questions/48796169/how-to-fix-ipykernel-launcher-py-error-unrecognized-arguments-in-jupyter
## testing
to test, make sure you have jupyter notebooks installed. you can do this by running:
```
pip install notebook
```
or following instructions from [here](https://jupyterlab.readthedocs.io/en/stable/getting_started/installation.html).
once installed, start a notebook (`jupyter notebook`), launch a python3 kernel and run the following snippet:
```python
import sys
# make sure this is the path for your system
sys.path.insert(0, "/path/to/bitcoin/test/functional")
from test_framework.test_shell import TestShell
test = TestShell().setup(num_nodes=2, setup_clean_chain=True)
```
you should see the following output, without errors:
if you are unfamiliar with notebooks, here is a short guide on using them: https://jupyter.readthedocs.io/en/latest/running.html
ACKs for top commit:
MarcoFalke:
review ACK 168b6c317ca054c1287c36be532964e861f44266
jamesob:
crACK https://github.com/bitcoin/bitcoin/pull/22201/commits/168b6c317ca054c1287c36be532964e861f44266
practicalswift:
cr ACK 168b6c317ca054c1287c36be532964e861f44266
Tree-SHA512: 4fee1563bf64a1cf9009934182412446cde03badf2f19553b78ad2cb3ceb0e5e085a5db41ed440473494ac047f04641311ecbba3948761c6553d0ca4b54937b4
|
|
Since m_next_addr_send is on a Poisson distribution, increase the mocktime bump
to ensure we don't experience flakiness in the tests. Closes #22243.
|
|
|
|
|
|
Add two simple helper functions to `AddrReceiver` to support callers currently
using `GetAddrStore` [used in next commit].
|
|
The `on_addr` functionality of `AddrReceiver` tests logic specific to how the
addr messages are set up in the test bodies. To allow other callers to also use
`AddrReceiver`, only apply the assertion logic if the caller indicates
desirability by setting `test_addr_contents` to true when initializing the
class.
|
|
testshell in jupyter was failing due to an extra arg.
this adds a dummy -f param, which allows TestShell to
be used in a command line or jupyter environment
|
|
and check tx vsize
d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0ef2dcd307023aaaab40373c8bf17db1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
|
|
time spent waiting
b9e76f1bf08c52fcd402b2314e00db4ad247ebc8 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7dc9a7b0c55d28011161606399417664 rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef698a1695050f82ef6567b3b87a21861 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59fe51473571661316214156fbdbdcae rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8
promag:
ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
|
|
|
|
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.
This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex)
-END VERIFY SCRIPT-
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
|
|
`FromHex` is mostly used for transactions, so we introduce a
shortcut `tx_from_hex` for `FromHex(CTransaction, hex_str)`.
|
|
30aee2dfe671b347438c1c327c6f79edfacff1ce tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4ded6116a42d2783c96c60ef0f255a1b2 Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
|
|
test_no_inherited_signaling RBF test
fa7d71f270b89c9d06230d4ff262646f9ea29f4a test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a4b02a41b7448b45f0e6cdfdbb53ac3 test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649e3da4a5a5f6cffac3fc748bb1ca900 test: Remove unused generate() from test (MarcoFalke)
faff3f35b778d9af3d649b303d7edab49bfe40b4 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups :art:
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a thanks for the explanations, nicely done
theStack:
ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a 🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
|
|
|
|
|
|
55311197c483477b79883da5da09f2bc71acc7cf Added new test for future blocks reacceptance (sanket1729)
511a5af4622915c236cfb11df5234232c2983e45 Fixed inconsistencies between code and comments (sanket1729)
Pull request description:
This Commit does 3 things:
1) Adds a test case for checking reacceptance a previously rejected block which
was too far in the future.
~~2) clean up uses of rehash or calc_sha256 where it was not needed~~
3) While constructing block 44, this commit makes the code consistent with the expected figure in
the comment just above it by adding a transaction to the block.
4) Fix comment describing `sign_tx()` function
ACKs for top commit:
duncandean:
reACK 5531119
brunoerg:
reACK 55311197c483477b79883da5da09f2bc71acc7cf
Tree-SHA512: d40c72fcdbb0b2a0715adc58441eeea08147ee2ec5e371a4ccc824ebfdc6450698bd40aaeecb7ea7bfdb3cd1b264dd821b890276fff8b8d89b7225cdd9d6b546
|
|
processes when using `--failfast`
451b96f7d2796d00eabaec56d831f9e9b1a569cc test: kill process group to avoid dangling processes (S3RK)
Pull request description:
This is an alternative to #19281
This PR fixes a problem when after test failure with `--failfast` option there could be dangling nodes. The nodes will continue to occupy rpc/p2p ports on the machine and will cause further test failures.
If there are any dangling nodes left at the end of the test run we kill the whole process group.
Pros: the operations is immediate and won't lead to CI timeout
Cons: the test_runner process is also killed and exit code is 137
Example output:
```
...
Early exiting after test failure
TEST | STATUS | DURATION
rpc_decodescript.py | ✓ Passed | 2 s
rpc_deprecated.py | ✓ Passed | 2 s
rpc_deriveaddresses.py | ✓ Passed | 2 s
rpc_dumptxoutset.py | ✖ Failed | 2 s
ALL | ✖ Failed | 8 s (accumulated)
Runtime: 4 s
Killed: 9
> echo $?
137
```
ACKs for top commit:
MarcoFalke:
review ACK 451b96f7d2796d00eabaec56d831f9e9b1a569cc
aitorjs:
ACK 451b96f7d2796d00eabaec56d831f9e9b1a569cc. Manual testing with and without **--failfast**.
Tree-SHA512: 87e510a1411b9e7571e63cf7ffc8b9a8935daf9112ffc0f069d6c406ba87743ec439808181f7e13cb97bb200fad528589786c47f0b43cf3a2ef0d06a23cb86dd
|
