It returns an incorrect result when called with a Decimal,
for which the "//" operator works differently.
Also drop unnecessary call to satoshi_round.
Github-Pull: #24239
Rebased-From: d1fab9d5d2
(cherry picked from commit 269553fe73b17f8acda3071a48836c66092d31d0)
NB: This code is commented out in elements and the cherry-pick is done
commenting out the code too.
Because of floating point precision issues, not all of the rounding done
is always correct. To fix this, the fee calculation for
assert_fee_amount is changed to better reflect how CFeeRate::GetFee does
it.
First the feerate is converted to an int representing sat/kvb. Then this
is multiplied by the transaction size, divivided by 1000, and rounded up
to the nearest sat. The result is then converted back to BTC (divided by
1e8) and then rounded down to the nearest sat to avoid precision errors.
Github-Pull: #22949
Rebased-From: 80dc829be7
(cherry picked from commit c768bfa08af034c744402d4294cc323d653b97b8)
Messy merge conflicts because this PR backported some ad-hoc stuff from
upstream while keeping a few things that upstream deleted. Hopefully
reviewing is easier than doing this in the first place, since ultimately
all I did was delete code from one side or another of the conflicts.
(Ok, I also changed some boost optional stuff to std::optional, and had
to patch up a test file for test framework changes.)
When reviewing the detailed crypto, bear in mind that taptweaks, like all
hashes are the kind of crypto that cannot be subtly wrong -- it will either
fail very hard or be correct. And we have independent implementations in
C++ and Python that cross-check each other, si it's unlikely to be the former.
This forward-ports the new Taproot sighash but does not fix a couple
22-blocked TODOs related to the MissingDataBehavior enum. Should be fixed
in a followup commit.
One nontrivial change I had to make was feeding the genesis hash to SignTransaction
(the "global" one in script/sign.cpp) so that it could correctly compute
the sighash at signing time.
Surprisingly easy to do. Almost all of the diff resolution was mechanically
* replacing boost::variant with std::variant
* replacing Optional with std::optional
* then replacing `nullopt` with `std::nullopt`
* updating the RPC functions for the new RPCArg::Default type
* update the tests/ directory to make new (since 22) tests use arrays for
createrawtransaction outputs
* other ad-hoc changes to function parameters etc (not too many of these)
I had to "really" change the code in PrecomputePSBTData, which was introduced
in 22.0 and affected by PSET, but this function was like 8 lines long so it
was easy.
Reviewing the diff may be a bit difficult because of the mix of mechanical
changes and ad-hoc things. Probably the most straightforward thing to do
is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff,
then diff the remaining conflicts against this commit.
TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
in documentation and help text even though it was deleted. Need to fix this
in 0.21 in a separate PR.
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
7593b06bd1 test: ensure I2P addresses are relayed (Vasil Dimov)
e7468139a1 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d2a4 test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
86742811ce test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02708 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)
Pull request description:
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.
Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
ACKs for top commit:
jonatack:
ACK 7593b06bd1
naumenkogs:
ACK 7593b06bd1.
laanwj:
Code review ACK 7593b06bd1
kristapsk:
ACK 7593b06bd1. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.
Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
14f93579a5 Add pegins and issuance test (sanket1729)
edf8455fe4 Fix bug in CAssetIssuance decoding (sanket1729)
cdd75d4251 Update OP_SUCCESS for elements with allowed opcodes (sanket1729)
4618097ab4 Implement taphash for elements (sanket1729)
6c985308d7 Implement Taphash in test framework (sanket1729)
a26f6fead6 Expose only blockchain hash twice in header (sanket1729)
afb9e7b727 taproot: feed genesis hash and parent pegged asset to sighash function (Andrew Poelstra)
Pull request description:
ACKs for top commit:
apoelstra:
ACK 14f93579a5
Tree-SHA512: f0a3f6ef9f8958bc948cc42375600767f39aadc0dac096fe8e7823dc69ddc4204360e504af7801d05bb19ae4e6ace6a271842531c192e330167381cf2690c856
What a pain in the ass. This changes the `feature_cltv` functional test
to use the `MiniWallet`, an internal Pythonic wallet that produces
"signatures" consisting of several OP_NOPs in a row, so that this test
can be used when the wallet is disabled.
This means that the functions in feature_cltv which add a CLTV check and
change the locktime/sequence number no longer need to re-sign when they
change the txid, so they don't...HOWEVER, our Elements-specific
`feature_block_v4` test calls these functions with a real transaction
and depended on the re-signing behavior.
After several attempts at min-effort min-diff fixes I landed on this,
which simply sets the locktime at transaction creation and avoids having
to re-sign at all.
Also, I had to change the number of NOPs in the fake wallet code to make
a vsize assertion pass, presumably because of differences in Bitcoin/Elements
transaction serialization.
This is part of the "UTXO Set Statistics" project in Core, which aims
to make things like "checking coin supply" faster. This stuff, and
related things in coinstats.cpp, have always been broken for Elements
in the sense that we consider only explicit outputs and we ignore
assets (so everything just gets added together to get a meaningless
total). It probably wouldn't be too hard to restrict this to only
consider policyAsset, but it's out of scope for a rebase IMO.
Also, I think this situation is fine .. I don't understand the motivation
for this or why Core is merging this when they refuse to merge an address
index .. but I guess we'll see if there are users who care about this data
and who care about it being meaningful on Elements.
Also, apologies for the big diff -- there were some mechanical changes
to deal with CT amounts, but most of the changes related to the difference
in how fees are accounted for. While I'm not thrilled with this PR, its
functional test is really good! So I think what I eventually came up with
is internally consistent.
