💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596597207)
Nit:
```suggestion
from decimal import Decimal, ROUND_DOWN
```

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596617286)
nit:

```suggestion
SATS_PRECISION = Decimal('0.00000001')
...

def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
"""Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
return Decimal(amount).quantize(SATS_PRECISION, rounding=rounding)
```

The `Decimal('0.00000001')` appears in a few places, would be good to extract it as a constant `SATS_PRECISION` (or whatever name you prefer) for readability and to make it less
...

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596607243)
Default `Decimal` rounding is [`ROUND_HALF_EVEN`](https://docs.python.org/3/library/decimal.html#decimal.ROUND_HALF_EVEN) -- "Round to nearest with ties going to nearest even integer.":

```python
>>> from decimal import getcontext
>>> getcontext().rounding
ROUND_HALF_EVEN
```

I wasn't super sure what you are trying to do here, but this example helps:

```python
>>> Decimal(0.004).quantize(Decimal('0.01'))
Decimal('0.00')

>>> Decimal(0.008).quantize(Decimal('0.01'))
Decimal('0.0
...

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596629322)
You mean in [mempool_package_limits](https://github.com/bitcoin/bitcoin/pull/21800/files#diff-153b2fda84a768c77135f65d26f04fa86262a89f4009f05690aeb47269a418e7)?

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596631691)
Hmm so not redundant keeping in mind that testing `getrawmempool` is also needed, got it.

👍 rkrux approved a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948#pullrequestreview-2049875497)
Thanks for quick responses.

💬 kristapsk commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2104444054)
Concept ACK

🤔 glozow reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)
Ok I have split the first commit into:
- net processing change to stop querying by txid
- 2 refactors changing the TxOrphanage methods to be by Wtxid only
- TxOrphanage change to index by wtxid and thus allow entries with the same txid

Hopefully in review, we can convince ourselves that the first commit is the right thing to do, separately from the other changes. The tests may help with that - also added one for invs that helps illustrate what the cases are, and added unit tests.

I thou
...

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596628218)
fixed thanks

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596626751)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596626910)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596628093)
inlined. didn't do the rename as I don't think it's incorrect and want to minimize diff

👍 vasild approved a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2049903727)
ACK 469e4834bad295f6d9fbf8048a8db23aa758bac9

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596649444)
Split up the commit, hope it helps (https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1596649820)
Adding 5 to the timeout fixes it for me. Modifying the line with the mocktime does not (I also tried removing that line which caused the test to fail again, proving its usefulness).
```
with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE + 5):
self.restart_node(0, extra_args=[f'-seednod
...

🤔 ismaelsadeeq reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2049928550)
Post merge Tested ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1596687155)
I feel we should tie this `only-path` here to `taproot_construct` call in `address.py`, either by passing it as a parameter or creating it as a constant. Otherwise, it makes you wonder here at this line where did `only-path` come from.

👍 rkrux approved a pull request: "test: fix MiniWallet script-path spend (missing parity bit in leaf version)"
(https://github.com/bitcoin/bitcoin/pull/30076#pullrequestreview-2049966561)
tACK [e4abac2](https://github.com/bitcoin/bitcoin/pull/30076/commits/e4abac2fce4af98108d899bc0480d066e5da7754)

Reviewing #29939 previously made it easy for me to review this one!

Make successful and all functional tests pass on this branch. Using the shared patch, I can see these 2 tests fails in master branch.

```
feature_versionbits_warning.py | ✖ Failed | 3 s
wallet_fundrawtransaction.py --descriptors | ✖ Failed | 17 s

ALL
...

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1596687535)
Would love to see the corresponding cpp reference code, share if you know?

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596696710)
Awesome, thanks @theuni ! I pulled this commit in and also add a "simple read only vector like interface" to the `KeyPair` class (same as `CKey`). Needed for: https://github.com/bitcoin/bitcoin/commit/5af5164b84b534c9a3171d898c09125957b21ba1#diff-e5cbd40011e760f175b82d7ec44ad512bdb3d9108068b660642204da9c9187b1R204

I also rebased #28122 on this PR and confirmed everything works with the new `KeyPair` class.