💬 Subhra264 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1287435261)
According to the BIP,
> Inputs not specifying a lock time field can take both types of lock times, as can those that specify both.

so if I am not missing anything here and both the `time_lock` and `height_lock` of `psbtin` are `std::nullopt`, shouldn't it be accepted irrespective of `input` lock times?

👍 stickies-v approved a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1567815521)
ACK fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125, really like this!

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287446839)
nit: using `.at(i)` here and `[i]` just a few lines up, when I think they both have the same preconditions, so staying consistent is probably better

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1287458787)
`std::vector<PackageEntry&>` isn't possible, but will do reference wrappers

🤔 instagibbs reviewed a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1543557301)
dropping the comments since I haven't seen any approach pushback yet, and I don't want to lose these comments accidentally

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272336717)
very glad to see the special casing of this to MISSING_INPUTS gone

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272306357)
yay! much easier to think about now

needs a reasonable comment to distinguish from TX_MEMPOOL_POLICY

my suggestion was adding something like "but result may differ when in different package"

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272371805)
We'll need some miniminer Experts(TM) to review this, but I'm just going to assume for now that this will be used as a topological "tie breaker" for linearization that is currently in the branch to this point

so if there's a bug in it, it's no worse than topo-sort, and can be improved later if dependencies need to be cut for whatever reason

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272419757)
> (a.fee * a.vsize > b.fee * a.vsize)

fix comment

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272355103)
"[refactor] fill in results for every tx in AcceptPackage"

It could have just been submitted in the above loop, maybe if this goes away later it won't be worth being more verbose...

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272429657)
would be nice to put "Filtered" in the function names of whatever is doing filtering to reduce mental load

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272491372)
Seems like this will continue evaluating individual txns that have no in-package ancestors(that aren't accepted individually before!). Seems safe, but wasn't immediately obvious to me. It will abort any *package* evaluation later.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272481752)
if this somehow happens in prod, do we want to keep stumbling along?

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272542996)
I think we decided pre-cluster mempool we can't really nail all these cases due to inherent symmetry.

With this rule we can see cases where the penultimate child is paid for by parents, the penultimate child and final child being immediately trimmed next at below-minfee rates. It's not very pathological since the evicted package is below minfee and if the mempool isn't too large it is worthwhile to mine. I think we agreed these can still exist, but just noting.

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287466821)
Hmm.

I haven't tried this using the c-i scripts, but running clang-tidy manually against current master, this works fine for me as-is:
```bash
$ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-tidy-16 --load=/home/cory/dev/bitcoin2/contrib/devtools/bitcoin-tidy/build2/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends
...

💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1670067500)
You'd likely have to go back to 2016 or before , when RBF wasn't added, to find the lists of merchants who accepted 0-conf transactions for smaller IRL transactions. A lot of merchants stopped accepting 0- conf when it was made easier to fraudulently cancel a transaction which has overall likely slowed down IRL merchant adoption and use, if not reversed it.



>Users can't make decision on their own

are you serious? "Bitcoin users are dumb and us devs know what's best for them" is a real
...

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287493545)
Ugh, I was inclined to argue with you because I think the tests should be as tight as possible. But realistically, it's more likely for a class to be forgotten in the checks (as has happened here) than a false-positive in some future class. So I begrudgingly agree.

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670094367)
I install in a we3 new wallet ,I have 2 unit 2 BTC in my profile I don't underestand your means that you haven't authorized to merge ,merge in a blockchain else just there unfortunately someone with now of forks of staking withdrawals my coins zBzTC legally me swhouse and get profit of staking of forks it hummm?! Truly interest

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670100363)
Why to offer you why I donot are authorized to merge this pull request tell me with now who use of my forks and staking my profile 2 unit BTC me?@'📈🤔🥵☹️ @

🤔 furszy reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1566336739)
Code review ACK 1e0f5f088.