💬 S3RK commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#discussion_r1156788080)
typo: to sign a transaction with the first ~~signature~~ key

💬 S3RK commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1495422350)
ACK 42107710a35dd4b489757dd9317251f4b27cd3be

💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1156844031)
I haven't written a test for this, so all the following is not verified and just my guess based on the code understanding.

This is not a regression in this particular PR, but this could lead to misleading errors in a situation when a proper solution exists, but our coin selection failed to find it and only found solution that exceeded weight.
Let's imagine following scenario:
- UTXO pool: two big coins 50BTC, a ton of 0.001BTC.
- Target amount: 90BTC
- Proper solution: just take two big c
...

💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1156851716)
I wrote #24580 at some point to capture known to me inefficiencies of current coin selection. The scenario I described above is very close to case no.3 in `test_one_big_and_many_small_coins`

👍 Sjors approved a pull request: "script: add description for the functionality of each opcode"
(https://github.com/bitcoin/bitcoin/pull/27109)
re-ACK 86b43d7fdd086c308f58414f3718a9cbf106bd05

💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156881801)
I see. In that case, just add what you just wrote as a comment.

💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156884391)
My main point was to clarify the comment "future, introduce a checkbox to customize this value". It could be done as source code comments or a Github issue.

💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1495532740)
ACK 68eed5df8656bed1be6526b014e58d3123102b03

Remaining feedback is only about source code comments.

💬 meglio commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#discussion_r1156887830)
Thank you, fixed.

👍 hebasto approved a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
ACK 24ef4bb6c2266e157008cbd2c394f9f83c7b5816, tested on Ubuntu 22.04.

Happy to re-ACK after fixing a [typo](https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156323737) :)

💬 hebasto commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1495607071)
Maybe add a check to `configure.ac` whether ignoring of `-Wgnu-zero-variadic-macro-arguments` is [really](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?

💬 fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156956396)
Thanks, fixed.

💬 fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1495616682)
Pushed to fix the typo.

🚀 fanquake merged a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)

👍 hebasto approved a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
re-ACK 436df1e826cae036caed3e983715a4ed4e441321

💬 fanquake commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1495674216)
Concept ACK

💬 Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#discussion_r1157016059)
Yeah, that does make more sense. I don't see any harm in automatically logging the asmap information.

💬 Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1495685701)
Concept ACK.

💬 dergoegge commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495695976)
Concept ACK

Personally, I have given up a little bit on fighting fingerprinting (at least I've de-prioritized it) because at this point there are so many fingerprinting techniques that we should probably either give up or figure out a way to avoid them ~entirely by design.

> we could advertise our onion address to clearnet peers

But this is such an obvious fingerprint that we should address it.

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495725877)
Or why not even expand this logic to all networks and delete `CNetAddr::GetReachabilityFrom()`? That is - advertise our X address only to peers from network X (for X being any of IPv4, IPv6, Tor, I2P, CJDNS).