Age | Commit message (Collapse) | Author |
|
with same txid
0fb17bf61a40b73a2b81a18e70b3de180c917f22 [log] updates in TxOrphanage (glozow)
b16da7eda76944719713be68b61f03d4acdd3e16 [functional test] attackers sending mutated orphans (glozow)
6675f6428d653bf7a53537bd773114f4fb5ba53f [unit test] TxOrphanage handling of same-txid-different-witness txns (glozow)
8923edfc1f12ebc6a074651c084ba7d249074799 [p2p] allow entries with the same txid in TxOrphanage (glozow)
c31f148166f01a9167d82501a77823785d28a841 [refactor] TxOrphanage::EraseTx by wtxid (glozow)
efcc5930175f31b685adb4627a038d9f0848eb1f [refactor] TxOrphanage::HaveTx only by wtxid (glozow)
7e475b9648bbee04f5825b922ba0399373eaa5a9 [p2p] don't query orphanage by txid (glozow)
Pull request description:
Part of #27463 in the "make orphan handling more robust" section.
Currently the main map in `TxOrphanage` is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we'll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid.
This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test.
Prior to #28970, we don't rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node's orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we'll end up accepting it. However, this is a bit more painful for 1p1c: it's easier for an attacker to tell when a tx is going to hit a node's orphanage, and we need to store the correct orphan + receive the parent before we'll consider the package. If we start out with a bad orphan, we can't evict it until we receive the parent + try the 1p1c, and then we'll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c.
ACKs for top commit:
AngusP:
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
itornaza:
trACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
instagibbs:
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
theStack:
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
sr-gi:
crACK [0fb17bf](https://github.com/bitcoin/bitcoin/pull/30000/commits/0fb17bf61a40b73a2b81a18e70b3de180c917f22)
stickies-v:
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22
Tree-SHA512: edcbac7287c628bc27036920c2d4e4f63ec65087fbac1de9319c4f541515d669fc4e5fdc30c8b9a248b720da42b89153d388e91c7bf5caf4bc5b3b931ded1f59
|
|
For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional<UniValue> with a default value of
UniValue::VNULL.
A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
|
|
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.
|
|
Only for JSON-RPC 2.0 requests.
|
|
|
|
We want to ensure that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents,
that we don't end up accepting packages we shouldn't.
|
|
- Add elapsed time in "remove orphan" log
- Add size in "stored orphan" log
- grammar edit
|
|
|
|
|
|
Adds error handling of invalid ports to rpcconnect and rpcport,
with associated functional tests.
|
|
in MempoolPackagesTest
e912717ff63f111d8f1cd7ed1fcf054e28f36409 test: add missing comparison of node1's mempool in MempoolPackagesTest (umiumi)
Pull request description:
#29941 Recreated a pull request because there was a conflict. Trying to resolve the conflict but the old one automatically closed.
Add missing comparison for TODO comments in `mempool_packages.py`
Also, notice that the ancestor size limits and descendant size limits actually implemented in #21800 , so I removed the todo for those two size limits.
ACKs for top commit:
kevkevinpal:
ACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)
achow101:
ACK e912717ff63f111d8f1cd7ed1fcf054e28f36409
alfonsoromanz:
Tested ACK e912717ff63f111d8f1cd7ed1fcf054e28f36409. The code looks good to me and the test execution is successful.
rkrux:
tACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)
Tree-SHA512: 8cb51746b0547369344c9ceef59599bfe9c91d424687af5e24dc6641f9e99fb433515d79c724e71fd3d5e02994f0cef623d3674367b8296b05c3c6fcdde282ef
|
|
fd6a7d3a13d89d74e161095b0e9bd3570210a40c test: use sleepy wait-for-log in reindex readonly (Matthew Zipkin)
Pull request description:
Also rename the busy wait-for-log method to prevent recurrence. See https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1532578152
ACKs for top commit:
maflcko:
utACK fd6a7d3a13d89d74e161095b0e9bd3570210a40c
achow101:
ACK fd6a7d3a13d89d74e161095b0e9bd3570210a40c
tdb3:
ACK for fd6a7d3a13d89d74e161095b0e9bd3570210a40c
rkrux:
ACK [fd6a7d3](https://github.com/bitcoin/bitcoin/pull/30006/commits/fd6a7d3a13d89d74e161095b0e9bd3570210a40c)
Tree-SHA512: 7ff0574833df1ec843159b35ee88b8bb345a513ac13ed0b72abd1bf330c454a3f9df4d927871b9e3d37bfcc07542b06ef63acef8e822cd18499adae8cbb0cda8
|
|
UTXO mixing, use in `fill_mempool`
dd8fa861939d5b8bdd844ad7cab015d08533a91a test: use tagged ephemeral MiniWallet instance in fill_mempool (Sebastian Falbesoner)
b2037ad4aeb4e16c7eb1e5756d0d1ee20172344b test: add MiniWallet tagging support to avoid UTXO mixing (Sebastian Falbesoner)
c8e6d08236ff225db445009bf513d6d25def8eb2 test: refactor: eliminate COINBASE_MATURITY magic number in fill_mempool (Sebastian Falbesoner)
4f347140b1a31237597dd1821adcde8bd5761edc test: refactor: move fill_mempool to new module mempool_util (Sebastian Falbesoner)
Pull request description:
Different MiniWallet instances using the same mode (either ADDRESS_OP_TRUE, RAW_OP_TRUE or RAW_P2PK) currently always create and spend UTXOs with identical output scripts, which can cause unintentional tx dependencies (see e.g. the discussion in https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565443465). In order to avoid mixing of UTXOs between instances, this PR introduces the possibility to provide a MiniWallet tag name, that is used to derive a different internal key for the taproot construction, leading to a different P2TR output script. Note that since we use script-path spending and only the key-path is changed here, no changes in the MiniWallet spending logic are needed.
The new tagging option is then used in the `fill_mempool` helper to create an ephemeral wallet for the filling txs, as suggested in https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565964264. To avoid circular dependencies, `fill_mempool` is moved to a new module `mempool_util.py` first.
I'm still not sure if a generic word like "tag" is the right term for what this tries to achieve, happy to pick up better suggestions. Also, maybe passing a tag name is overkill and a boolean flag like "random_output_script" is sufficient?
ACKs for top commit:
glozow:
ACK dd8fa861939
achow101:
ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a
rkrux:
tACK [dd8fa86](https://github.com/bitcoin/bitcoin/pull/29939/commits/dd8fa861939d5b8bdd844ad7cab015d08533a91a)
brunoerg:
utACK dd8fa861939d5b8bdd844ad7cab015d08533a91a
Tree-SHA512: 5ef3558c3ef5ac32cfa79c8f751972ca6bceaa332cd7daac7e93412a88e30dec472cb041c0845b04abf8a317036d31ebddfc3234e609ed442417894c2bdeeac9
|
|
updates comment in ConsiderEviction
d53d84834747c37f4060a9ef379e0a6b50155246 test: adds outbound eviction tests for non outbound-full-relay peers (Sergi Delgado Segura)
a8d9a0edc7cef2c31a557ef53eb45520976b0d65 test: adds outbound eviction functional tests, updates comment in ConsiderEviction (Sergi Delgado Segura)
Pull request description:
## Motivation
While checking the outbound eviction code I realized a case was not considered within the comments, which in turn made me realize we had no functional tests for the outbound eviction case (when I went to check/add the test case).
This PR updates the aforementioned comment and adds functional tests to cover the outbound eviction logic, in addition to the existing unit tests found at `src/test/denialofservice_tests.cpp`.
ACKs for top commit:
davidgumberg:
reACK https://github.com/bitcoin/bitcoin/commit/d53d84834747c37f4060a9ef379e0a6b50155246
tdb3:
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
achow101:
ACK d53d84834747c37f4060a9ef379e0a6b50155246
cbergqvist:
ACK d53d84834747c37f4060a9ef379e0a6b50155246
Tree-SHA512: 633b84bb1229fe21e2f650c1beada33ca7f190b64eafd64df2266516d21175e5d652e019ff7114f00cb8bd19f5817dc19e65adf75767a88e24dc0842ce40c63e
|
|
|
|
a snapshot twice
b259b0e8d360726b062c4b0453d1cf5a68e1933f [Test] Assumeutxo: ensure failure when importing a snapshot twice (Alfonso Roman Zubeldia)
Pull request description:
I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate.
ACKs for top commit:
fjahr:
Code review ACK b259b0e8d360726b062c4b0453d1cf5a68e1933f
kevkevinpal:
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)
achow101:
ACK b259b0e8d360726b062c4b0453d1cf5a68e1933f
rkrux:
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)
Tree-SHA512: 3510861390d0e40cdad6861b728df04827a1b63e642f3d956aee66ed2770b1cb7e3aa3eb00c62eb9da0544703c943cc5296936c9ebfcac18c719741c354421bb
|
|
other improvements
78e52f663f3e3ac86260b913dad777fd7218f210 doc: rpc: fix submitpackage examples (stickies-v)
1a875d4049574730d4a53a1b68bd29b80ad96d38 rpc: update min package size error message in submitpackage (stickies-v)
f9ece258aa868d0776caa86b94e71ba05a9b287e doc: rpc: submitpackage takes sorted array (stickies-v)
17f74512f0d19cb452ed79a4bff5a222fcdb53c4 test: add bounds checking for submitpackage RPC (stickies-v)
Pull request description:
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.
Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplication and sending a confusing error message.
- fixups to the `submitpackage` examples
ACKs for top commit:
fjahr:
re-ACK 78e52f663f3e3ac86260b913dad777fd7218f210
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/29292/commits/78e52f663f3e3ac86260b913dad777fd7218f210
achow101:
ACK 78e52f663f3e3ac86260b913dad777fd7218f210
glozow:
utACK 78e52f663f3e3ac86260b913dad777fd7218f210
Tree-SHA512: a8845621bb1cbf784167fc7c82cb8ceb105868b65b26d3465f072d1c04ef3699e85a21a524ade805d423bcecbc34f7d5bff12f2c21cbd902ae1fb154193ebdc9
|
|
357ad110548d726021547d85b5b2bfcf3191d7e3 test: Handle functional test disk-full error (Brandon Odiwuor)
Pull request description:
Fixes: https://github.com/bitcoin/bitcoin/issues/23099
Handle disk-full more gracefully in functional tests
ACKs for top commit:
itornaza:
re-ACK 357ad110548d726021547d85b5b2bfcf3191d7e3
achow101:
reACK 357ad110548d726021547d85b5b2bfcf3191d7e3
cbergqvist:
reACK 357ad110548d726021547d85b5b2bfcf3191d7e3. Looks good!
tdb3:
re ACK for 357ad110548d726021547d85b5b2bfcf3191d7e3
Tree-SHA512: 9bb0d3fbe84600c88873b9f55d4b5d1443f79ec303467680c301be2b4879201387f203d9d1984169461f321037189b5e10a6a4b9d61750de638f072d2f95d77e
|
|
specific error messages
98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Eunovo:
Tested ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
achow101:
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
|
|
not open file
ee67bba76cca2355541f99bb731f58479981b29e test: added test coverage to loadtxoutset (kevkevin)
Pull request description:
The functional test coverage did not cover the rpc error of "Couldn't open file..." for loadtxoutset and this test adds coverage for it
This adds coverage to this line
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2777
ACKs for top commit:
maflcko:
ACK ee67bba76cca2355541f99bb731f58479981b29e
davidgumberg:
LGTM ACK https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e
rkrux:
ACK [ee67bba](https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e)
alfonsoromanz:
ACK ee67bba76cca2355541f99bb731f58479981b29e. Code looks good to me. I also ran `test/functional/feature_assumeutxo.py` to make sure all tests passes, including this one.
tdb3:
ACK for ee67bba76cca2355541f99bb731f58479981b29e
Tree-SHA512: 210a7eb928f625d2a8d9acb63ee83cb4aaec9c267e5a0c52ad219c2935466e2cdc68667e30ad29566e6060981587e5bec42805d296f6e60f9b3b13f3330575f2
|
|
4b9f49da2b120e81516ddc3dc577d7a2e58e02d3 doc: fix broken relative md links (willcl-ark)
Pull request description:
These relative links in our documentation are broken, fix them.
ACKs for top commit:
maflcko:
ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3
ryanofsky:
Code review ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3. Thanks for the updates!
ismaelsadeeq:
Re ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3
Tree-SHA512: df4ef5ddece6c21125ce719ed6a4f69aba4f884c353ff7a8445ecb6438ed6bf0ff8268a1ae19cdd910adaadc189c6861c445b4d469f92ee81874d810dcbd0846
|
|
keep to bitcoin-config.h includes
fa09451f8e6799682d7e7c863f25334fd1c7dce3 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba8267dea11a3eb03d5cf8b51dbb99be5d scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
TheCharlatan:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
hebasto:
re-ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's!struct.pack\(.<?B., (.*)\)!\1.to_bytes(1, "little")!g' $( git grep -l struct.pack )
sed -i --regexp-extended 's!struct.pack\(.<I., (.*)\)!\1.to_bytes(4, "little")!g' $( git grep -l struct.pack )
sed -i --regexp-extended 's!struct.pack\(.<H., (.*)\)!\1.to_bytes(2, "little")!g' $( git grep -l struct.pack )
sed -i --regexp-extended 's!struct.pack\(.<i., (.*)\)!\1.to_bytes(4, "little", signed=True)!g' $( git grep -l struct.pack )
sed -i --regexp-extended 's!struct.pack\(.<q., (.*)\)!\1.to_bytes(8, "little", signed=True)!g' $( git grep -l struct.pack )
sed -i --regexp-extended 's!struct.pack\(.>H., (.*)\)!\1.to_bytes(2, "big")!g' $( git grep -l struct.pack )
-END VERIFY SCRIPT-
|
|
This is done in prepration for the scripted diff, which can not deal
with those lines.
|
|
* Add () around some int values
* Remove b-prefix from strings
This is needed for the scripted diff to work.
|
|
Currently, the only allowed package topology has a min size of 2.
Update the error message to reflect that.
|
|
|
|
The functional test coverage did not cover the rpc error of Couldn't
open file for loadtxoutset and this test adds coverage for it
|
|
just a single one
42fb5311b19582361409d65c6fddeadbee14bb97 rpc: return warnings as an array instead of just a single one (stickies-v)
Pull request description:
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.
Fix that by returning all warnings as an array.
As a side benefit, clean up the GetWarnings() logic.
Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.
---
Some historical context from git log:
- when `GetWarnings` was introduced in 401926283a200994ecd7df8eae8ced8e0b067c46, it was used in the `getinfo` RPC, where only a [single error/warning was returned](https://github.com/bitcoin/bitcoin/commit/401926283a200994ecd7df8eae8ced8e0b067c46#diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c882396e1776b554878d2784b6b7391, with the description [stating](https://github.com/bitcoin/bitcoin/commit/ef2a3de25c882396e1776b554878d2784b6b7391#diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411) that it returned "any network warnings" but in practice still only a single warning was returned
ACKs for top commit:
achow101:
re-ACK 42fb5311b19582361409d65c6fddeadbee14bb97
tdb3:
Re ACK for 42fb5311b19582361409d65c6fddeadbee14bb97
TheCharlatan:
ACK 42fb5311b19582361409d65c6fddeadbee14bb97
maflcko:
ACK 42fb5311b19582361409d65c6fddeadbee14bb97 🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
|
|
|
|
Note that this commit doesn't change behaviour yet, as tagging isn't
used in any MiniWallet instance.
|
|
|
|
This is needed to avoid circular dependencies in later commits.
Can be reviewed via `--color-moved=dimmed-zebra`.
|
|
|
|
This exercises the bug fixed by previous commits, where
we were unable to generate and sign for segwit redeem scripts
(in this case multisig redeem scripts) longer than 520 bytes.
and also, this adds coverage for legacy 15-15 multisig script
generation and signing.
|
|
Move-only commit. No behavior change.
|
|
'test_mixing_uncompressed_and_compressed_keys'
And also, simplified the test a bit by re-using the already existing 'wallet_multi'
(instead of creating a new one). Plus, removed the 'is_bdb_compiled()' calls
which were there basically to check if the test has the wallet compiled or not.
|
|
The function exists merely to check that the node2's wallet
received the transactions created during all the 'do_multisig()'
calls.
It was created as a standalone function because 'getbalance()'
only returns something when transactions are confirmed. So,
the rationale on that time was to have a method mining blocks
to confirm the recently created transactions to be able to
check the incoming balance.
This is why we have the "moved" class field.
This change removes all the hardcoded amounts and verifies
node2 balance reception directly inside 'do_multisig()'.
|
|
Cleaning up the test in the following ways:
* Generate priv-pub key pairs used for testing only once (instead of doing it 4 times).
* Simplifies 'wmulti' wallet creation, load and unload process.
* Removes confusing class members initialized and updated inside a nested for-loop.
* Simplifies do_multisig() outpoint detection:
The outpoint index information is already contained in MiniWallet's
`send_to` return value dictionary as "sent_vout".
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
|
|
There is no need to manually initialize the wallets within the test
case. The test framework already initializes them when `_requires_wallet`
is true.
|
|
MAX_SCRIPT_ELEMENT_SIZE
ffc674595cb19b2fdc5705b355bdd3e7f724b860 Replace remaining "520" magic numbers with MAX_SCRIPT_ELEMENT_SIZE (Jon Atack)
Pull request description:
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
ACKs for top commit:
instagibbs:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
sipa:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
achow101:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
glozow:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number
Tree-SHA512: 462afc1c64543877ac58cb3acdb01d42c6d08abfb362802f29f3482d75401a2a8adadbc2facd222a9a9fefcaab6854865ea400f50ad60bec17831d29f7798afe
|
|
These relative links in our documentation are broken, fix them.
|
|
|
|
base height & amount > MAX_MONEY supply
ec1f1abfefa281e62bb876aa1c4738d576ef9a47 test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply (jrakibi)
Pull request description:
### Ensure snapshot loading fails for coins exceeding base height
**Objective**: This test verifies that snapshot loading is correctly rejected for coins with a height greater than the base height.
**Update**:
- Added `test_invalid_snapshot_wrong_coin_code` to `feature_assumeutxo.py`.
- The test artificially sets a coin's height above 299 in a snapshot and checks for load failure.
- Edit: Added a test case for outputs whose amounts surpass the MAX_MONEY supply limit.
This implementation addresses the request for enhancing `assumeutxo` testing as outlined in issue #28648
---
**Edit: This is an explanation on how I arrive at content values: b"\x84\x58" and b"\xCA\xD2\x8F\x5A"**
You can use this tool to decode the utxo snapshot https://github.com/jrakibi/utxo-live
Here’s an overview of how it’s done:
The serialization format for a UTXO in the snapshot is as follows:
1. Transaction ID (txid) - 32 bytes
2. Output Index (outnum)- 4 bytes
3. VARINT (code) - A varible-length integer encoding the height and whether the transaction is a coinbase. The format of this VARINT is (height << 1) | coinbase_flag.
4. VARINT (amount_v) - A variable-length integer that represents a compressed format of the output amount (in satoshis).
For the test cases mentioned:
* **`b"\x84\x58"`** - This value corresponds to a VARINT representing the height and coinbase flag. Once we decode this code, we can extract the height and coinbase using `height = code_decoded >> 1` and `coinbase = code_decoded & 0x01`. In our case, with code_decoded = 728, it results in `height = 364` and `coinbase = 0`.
* **`b"\xCA\xD2\x8F\x5A"`** - This byte sequence represents a compressed amount value. The decompression function takes this value and translates it into a full amount in satoshis. In our case, the decompression of this amount translates to a number larger than the maximum allowed value of coins (21 million BTC)
ACKs for top commit:
fjahr:
re-ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
maflcko:
ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a4 👑
achow101:
ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
Tree-SHA512: 42b36fd1d76e9bc45861028acbb776bd2710c5c8bff2f75c751ed505995fbc1d4bc698df3be24a99f20bcf6a534615d2d9678fb3394162b88133eaec88ca2120
|
|
|
|
LF) not to be used
fa9be2f79520fff9cfe2ed35ace05cb322680af3 lint: [doc] Clarify Windows line endings (CR LF) not to be used (MarcoFalke)
Pull request description:
It has been this case since the linter was introduced years ago. Given a misunderstanding (https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2088028856), clarify the docs.
ACKs for top commit:
brunoerg:
ACK fa9be2f79520fff9cfe2ed35ace05cb322680af3
Tree-SHA512: be714db9df533e0962ed84102ffdb72717902949b930d58cf5a79cba36297f6b2b1f75e65a2c1f46bcb8e2f4ad5d025f3d15210f468a5ec9631a580b74f923ea
|
|
e504b1fa1fa4d014b329dea81bfdf1aa55db238f test: Add test case for spending bare multisig (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/29113
ACKs for top commit:
ajtowns:
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine
maflcko:
utACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
achow101:
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
willcl-ark:
reACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
Tree-SHA512: 641a12599efa34e1a3eb65b125318df326628fef3e6886410ea9e63a044664fad7bcad46d1d6f41ddc59630746b9963cedb569c2682b5940b32b9225883da8f2
|
|
The RPC documentation for `getblockchaininfo`, `getmininginfo` and
`getnetworkinfo` states that "warnings" returns "any network and
blockchain warnings". In practice, only a single warning is returned.
Fix that by returning all warnings as an array.
As a side benefit, cleans up the GetWarnings() logic.
|
|
Also, remove the no longer needed, remaining definitions and checks of
HAVE_CONFIG_H.
|