8c3ff7d52a test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563825 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8c rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e67 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52a 🗂
kristapsk:
ACK 8c3ff7d52a
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
There are quite a few places that assume CalculateMemPoolAncestors
will return a value without raising an error. This helper function
adds logging (and Assume for debug builds) that ensures robustness
but increases visibility in case of unexpected failures
21ffe4b22a Merge bitcoin-core/secp256k1#1055: Prepare initial release
e025ccdf74 release: prepare for initial release 0.2.0
6d1784a2e2 build: add missing files to EXTRA_DIST
8c949f56da Merge bitcoin-core/secp256k1#1173: Don't use compute credits for now
13bf1b6b32 changelog: make order of change types match keepachangelog.com
b1f992a552 doc: improve release process
7e5b22684f Don't use compute credits for now
ad39e2dc41 build: change package version to 0.1.0-dev
5c789dcd73 Merge bitcoin-core/secp256k1#1168: Replace deprecated context flags with NONE in benchmarks and tests
d6dc0f4ae3 tests: Switch to NONE contexts in module tests
0c8a5caddd tests: Switch to NONE contexts in tests.c
86540e9e1f tests: add test for deprecated flags and rm them from run_context
caa0ad631e group: add gej_eq_var
37ba744f5b tests: Switch to NONE contexts in exhaustive and ctime tests
8d7a9a8eda benchmarks: Switch to NONE contexts
90618e9263 doc: move CHANGELOG from doc/ to root directory
e3f84777eb Merge bitcoin-core/secp256k1#1126: API cleanup with respect to contexts
4386a2306c examples: Switch to NONE contexts
7289b51d31 docs: Use doxygen style if and only if comment is user-facing
e7d0185c90 docs: Get rid of "initialized for signing" terminology
06126364ad docs: Tidy and improve docs about contexts and randomization
e02d6862bd selftest: Expose in public API
e383fbfa66 selftest: Rename internal function to make name available for API
d2c6d48de3 tests: Use new name of static context
53796d2b24 contexts: Rename static context
72fedf8a6c docs: Improve docs for static context
316ac7625a contexts: Deprecate all context flags except SECP256K1_CONTEXT_NONE
477f02c4de Merge bitcoin-core/secp256k1#1165: gitignore: Add *.sage.py files autogenerated by sage [skip ci]
092be61c5e gitignore: Add *.sage.py files autogenerated by sage
1a553ee8be docs: Change signature "validation" to "verification"
ee7341fbac docs: Never require a verification context
751c4354d5 Merge bitcoin-core/secp256k1#1152: Update macOS image for CI
2286f80902 Merge bitcoin-core/secp256k1#993: Enable non-experimental modules by default
e40fd277b7 Merge bitcoin-core/secp256k1#1156: Followups to int128_struct arithmetic
99bd335599 Make int128 overflow test use secp256k1_[ui]128_mul
a8494b02bf Use compute credits for macOS jobs
3afce0af7c Avoid signed overflow in MSVC AMR64 secp256k1_mul128
c0ae48c995 Update macOS image for CI
9b5f589d30 Heuristically decide whether to use int128_struct
63ff064d2f int128: Add test override for testing __(u)mulh on MSVC X64
f2b7e88768 Add int128 randomized tests
6138d73be4 Merge bitcoin-core/secp256k1#1155: Add MSan CI jobs
ddf2b2910e Merge bitcoin-core/secp256k1#1000: Synthetic int128 type.
86e3b38a4a Merge bitcoin-core/secp256k1#1149: Remove usage of CHECK from non-test file
00a42b91b3 Add MSan CI job
44916ae915 Merge bitcoin-core/secp256k1#1147: ci: print env to allow reproducing the job outside of CI
c2ee9175e9 Merge bitcoin-core/secp256k1#1146: ci: prevent "-v/--version: not found" irrelevant error
e13fae487e Merge bitcoin-core/secp256k1#1150: ci: always cat test_env.log
a340d9500a ci: add int128_struct tests
dceaa1f579 int128: Tidy #includes of int128.h and int128_impl.h
2914bccbc0 Simulated int128 type.
6a965b6b98 Remove usage of CHECK from non-test file
5c9f1a5c37 ci: always cat all logs_snippets
49ae843592 ci: mostly prevent "-v/--version: not found" irrelevant error
4e54c03153 ci: print env to allow reproducing the job outside of CI
a43e982bca Merge bitcoin-core/secp256k1#1144: Cleanup `.gitignore` file
f5039cb66c Cleanup `.gitignore` file
798727ae1e Revert "Add test logs to gitignore"
41e8704b48 build: Enable some modules by default
694ce8fb2d Merge bitcoin-core/secp256k1#1131: readme: Misc improvements
88b00897e7 readme: Fix line break
78f5296da4 readme: Sell "no runtime dependencies"
ef48f088ad readme: Add IRC channel
9f8a13dc8e Merge bitcoin-core/secp256k1#1128: configure: Remove pkgconfig macros again (reintroduced by mismerge)
cabe085bb4 configure: Remove pkgconfig macros again (reintroduced by mismerge)
3efeb9da21 Merge bitcoin-core/secp256k1#1121: config: Set preprocessor defaults for ECMULT_* config values
6a873cc4a9 Merge bitcoin-core/secp256k1#1122: tests: Randomize the context with probability 15/16 instead of 1/4
17065f48ae tests: Randomize the context with probability 15/16 instead of 1/4
c27ae45144 config: Remove basic-config.h
da6514a04a config: Introduce DEBUG_CONFIG macro for debug output of config
63a3565e97 Merge bitcoin-core/secp256k1#1120: ecmult_gen: Skip RNG when creating blinding if no seed is available
d0cf55e13a config: Set preprocessor defaults for ECMULT_* config values
55f8bc99dc ecmult_gen: Improve comments about projective blinding
7a86955800 ecmult_gen: Simplify code (no observable change)
4cc0b1b669 ecmult_gen: Skip RNG when creating blinding if no seed is available
af65d30cc8 Merge bitcoin-core/secp256k1#1116: build: Fix #include "..." paths to get rid of further -I arguments
40a3473a9d build: Fix #include "..." paths to get rid of further -I arguments
43756da819 Merge bitcoin-core/secp256k1#1115: Fix sepc256k1 -> secp256k1 typo in group.h
069aba8125 Fix sepc256k1 -> secp256k1 typo in group.h
accadc94df Merge bitcoin-core/secp256k1#1114: `_scratch_destroy`: move `VERIFY_CHECK` after invalid scrach space check
cd47033335 Merge bitcoin-core/secp256k1#1084: ci: Add MSVC builds
1827c9bf2b scratch_destroy: move VERIFY_CHECK after invalid scrach space check
49e2acd927 configure: Improve rationale for WERROR_CFLAGS
8dc4b03341 ci: Add a C++ job that compiles the public headers without -fpermissive
51f296a46c ci: Run persistent wineserver to speed up wine
3fb3269c22 ci: Add 32-bit MinGW64 build
9efc2e5221 ci: Add MSVC builds
2be6ba0fed configure: Convince autotools to work with MSVC's archiver lib.exe
bd81f4140a schnorrsig bench: Suppress a stupid warning in MSVC
09f3d71c51 configure: Add a few CFLAGS for MSVC
3b4f3d0d46 build: Reject C++ compilers in the preprocessor
1cc0941414 configure: Don't abort if the compiler does not define __STDC__
cca8cbbac8 configure: Output message when checking for valgrind
1a6be5745f bench: Make benchmarks compile on MSVC
git-subtree-dir: src/secp256k1
git-subtree-split: 21ffe4b22a9683cf24ae0763359e401d1284cc7a
Do not create strings and compare them to check if one `addr:port`
equals another. Use `CService::operator==()` instead.
`strDefaultProxyGUI` was assigned the same value 3 times. Instead save
it in `const CService ui_proxy` at the beginning of the function.
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
Both methods do the same thing, so simplify to having just one.
Further, `CService` inherits `CNetAddr` and `CService::ToString()`
overrides `CNetAddr::ToString()` but the latter is not virtual which
may be confusing. Avoid such a confusion by not having non-virtual
methods with the same names in inheritance.
"IP" stands for "Internet Protocol".
"IP address" is sometimes shortened to just "IP" or "address".
However, Tor or I2P addresses are not "IP addresses", nor "IPs".
Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or
I2P addresses:
`CService::ToStringIPPort()` -> `CService::ToStringAddrPort()`
`CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()`
-BEGIN VERIFY SCRIPT-
sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src)
sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src)
-END VERIFY SCRIPT-
956c67059c refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)
Pull request description:
This picks up the last commit from #19843.
Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
the version handshake is finished.
There are a couple of differences:
1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.
Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.
ACKs for top commit:
stratospher:
ACK 956c670
naumenkogs:
ACK 956c67059c
amitiuttarwar:
reACK 956c67059c
Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
- memcmp -> std::memcmp
8f5c560e11 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)
Pull request description:
Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)
ACKs for top commit:
stickies-v:
re-ACK [8f5c560](8f5c560e11)
Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
293849a260 univalue: Remove confusing getBool method (Ryan Ofsky)
Pull request description:
Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.
The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.
These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR.
ACKs for top commit:
justinpickering:
ACK 293849a260
sipa:
utACK 293849a260
w0xlt:
ACK 293849a260
hebasto:
ACK 293849a260, also verified that the removed `getBool` method is not mentioned in any docs:
furszy:
ACK 293849a2
Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
We need to check that the fee is not negative even before it is
finalized. The setting of fees for SFFO may adjust the fee to be
"correct" and no longer negative, but erroneously reduce the amounts too
far. So we need to check this condition before we do those adjustments.
It doesn't make sense to be checking whether the fee paid is underpaying
before we've finished setting the fees. So do that after we have done
the reduction for SFFO and change adjustment for fee overpayment.
The logest running tests should be at the front of the list in
test_runner.py. Since compiling with --enable-debug can have a
significant effect on test runtime, the order is based on the runtime
with that option configured.
test_submit_child_with_parents creates a p2p connection which waits for
the node to announce transactions to it. By whitelisting this
connection, we can reduce the amount of time spent waiting for this
announcement which improves the test runtime and runtime variance.
The work queue exceeded test in interface_rpc.py would repeatedly call
an RPC until the error was achieved. However hitting this error is
dependent on the processing speed of the computer and the optimization
level of the binary. Configurations that result in slower processing
would result in the RPC used being processed before the error could be
hit, resulting the test's runtime having a high variance.
Switching the RPC to waitfornewblock allows it to run in a much more
consistent time that is still fairly fast. waitfornewblock forces
the RPC server to allocate a thread and wait, occupying a spot in the
work queue. This is perfect for this test because the slower the RPC,
the more likely we will achieve the race condition necessary to pass the
test. Using a timeout of 500 ms appears to work reliably without causing
the test to take too long.
The sigops draining script in feature_taproot's block_submit was
initialized with a list that would end up always being iterated by
CScript's constructor. Since this list is very large, a lot of time
would be wasted. By creating and passing a bytes object initialized from
that list, we can avoid this iteration and dramatically improve the
runtime of feature_taproot.
feature_fee_estimation has a lot of loops that hit the RPC many times in
succession in order to setup scenarios. Using batched requests for these
can reduce the test's runtime without effecting the test's behavior.
Generating blocks is slow, especially when --enable-debug. There is no
need to generate a new block for each transaction, so avoid doing that
to improve this test's runtime.
fa7d71accc test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke)
fa933d6985 test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke)
Pull request description:
The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.
ACKs for top commit:
fanquake:
ACK fa7d71accc
pablomartin4btc:
re-ACK fa7d71accc
Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
Replacing `install` with `install-lib` and `install-bin` is not strictly
necessary just to update the library, but it takes advantage of recent
changes in the new version, and makes the build more minimal.
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.
The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.
These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fabb24cbef test: Use last release in compatibility tests (MarcoFalke)
Pull request description:
In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.
ACKs for top commit:
Sjors:
utACK fabb24c
Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
affbf58a1e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c97 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.
Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.
The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).
The bug is well described in bitcoin/bitcoin#22719.
Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).
Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
```sh
$ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
$ echo $?
1
```
Using the `export` command is a trivial solution:
```sh
$ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
foo
bar
end
$ echo $?
0
```
As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.
Fixesbitcoin/bitcoin#22719.
---
Also this PR removes no longer needed crutch from `qt.mk`.
ACKs for top commit:
fanquake:
ACK affbf58a1e
Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
fa579f3063 refactor: Pass reference to last header, not pointer (MacroFake)
Pull request description:
It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
ACKs for top commit:
john-moffett:
ACK fa579f3
aureleoules:
ACK fa579f3063
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
07dfbb5bb8 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès)
Pull request description:
Fixes#22189.
The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables.
ACKs for top commit:
jamesob:
ACK 07dfbb5bb8 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a))
theStack:
Concept and code-review ACK 07dfbb5bb8
Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
