💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569085792)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

Calling `ToUint256()` shouldn't be needed

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569100856)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

I'm struggling to see the usefulness of this. You are showing that, for the provided transactions, the ordering may be different based on the representation used (`wtxid`/`txid`/`ToUint256`/`GetHex`), but I don't think this clearly shows that the package hash is using one or the other.

You are already proving that the order is the one you are expecting by manually computing `calculated_hash_456`. You could also create different orderings based o
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568990742)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

I don't see what the point of building the `txid` from a literal in this way and comparing it to the one obtained via `GetHash()` is. I'm guessing you're trying to make the point that the txids are actually what you are claiming them to be (as opposed to just writing them in a comment), so the reader can manually check the difference between internal and human-readable lexicographic ordering. Is that really necessary?

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569200313)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

nit: I think it wouldn't hurt to have a comment here along the lines of:

/// Check that `GetPackageHash`/ `GetCombinedHash` are consistent with each other, and that the input order does not affect the resulting hash

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569086405)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

Same as before, no need to cast to `ToUint256`

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567870936)
In: 54d0c78d104fb5412a194816590a06cad8cadf80

This seems redundant

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569461340)
in 91f4efa420958a93f4620379f8830231f276b23b

This should also be comparable by `CTransactionRef`, shouldn't it? So `GetWitnessHash` doesn't need to be called here and in following loops

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569471367)
in: 91f4efa420958a93f4620379f8830231f276b23b

nit: Use `contains` instead of `count(...) > 0` for consistency with the previous check (previous loop)

👍 pinheadmz approved a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2006844953)
ACK 9c50b2fe44e4744204c51b82e86174db1d749cfe

Reviewed each commit and left some questions below. Built and ran all unit and functional tests on arm/macos. I am already familiar with this feature from #29415 and have run those tests as well.

One nitty nitty nit nit: commit message 9a9b545c7fc044242c4dd8a4916b2ba13b3ea850 uses c++ syntax instead of python ;-)


<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 9c50b2fe44e4744204c51b
...

💬 pinheadmz commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1569322866)
https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

any reason for this particular buffer size? some quick research suggests that 4kB and 8kB are common values for this, I'm just wondering if it really affects us.

💬 pinheadmz commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1569436155)
https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

Instead of the counter, you could also use `dest = self.destiantions.pop()` below and then log your warning once the array is empty

💬 pinheadmz commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1569289481)
https://github.com/bitcoin/bitcoin/pull/29420/commits/9a9b545c7fc044242c4dd8a4916b2ba13b3ea850

Why do we need to replace these values?
I changed this to:
```python
assert self.dstaddr == them[0]
assert self.dstport == them[1]
```
...and all the p2p tests still passed

💬 pinheadmz commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1569437411)
https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

had to read the docs on this one but I agree it is correct 👍

🤔 alfonsoromanz reviewed a pull request: "Bugfix - don't allow multiple dialogs for same tx in TransactionView"
(https://github.com/bitcoin-core/gui/pull/817#pullrequestreview-2007054657)
Tested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18. Only one dialog is opened for a single tx.

💬 alfonsoromanz commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#discussion_r1569483043)
The dialog is not brought to the foreground in my case (I am using Mac). In order to make it work I had to call `raise()` instead. Thoughts?

```suggestion
dlg->raise()
```

🤔 mzumsande reviewed a pull request: "test: Fix intermittent issue in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/pull/29898#pullrequestreview-2007043243)
Concept ACK

💬 mzumsande commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1569470049)
I think it would be more stable to have a global counter, so that `peer_id` is never reused. Currently, it is still reused over different `test_desirable_service_flags` calls.
E.g., if I add the net sleep and also change `CONNECTION_TYPES = ["outbound-full-relay"]` so that `test_desirable_service_flags` only makes one connection, I run into the timeout even with your fix.

🤔 glozow reviewed a pull request: "fuzz: explicitly cap the vsize of RBFs for diagram checks"
(https://github.com/bitcoin/bitcoin/pull/29879#pullrequestreview-2007103312)
ACK 016ed248ba0ae64e3f0c93bb47a2cd9b5e49cd85

💬 Joudyadam commented on pull request "deploy: remove some tools when cross-compiling for macOS":
(https://github.com/bitcoin/bitcoin/pull/29890#issuecomment-2062469035)
how can i withdrawl my 3500 USDT from okx wallet to binance please help me.
12 wallet phrase:

family nature fashion project scrub obscure bus crop coconut ship person winner

💬 Joudyadam commented on pull request "[26.x] archive 26.1 release notes + backports":
(https://github.com/bitcoin/bitcoin/pull/29899#issuecomment-2062471732)
how can i withdrawl my 3500 USDT from okx wallet to binance please help me.
12 wallet phrase:

family nature fashion project scrub obscure bus crop coconut ship person winner