💬 glozow commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934781088)
Left as is for now

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2623149705)
@sipa and @laanwj pinging you here as this PR is based on your comments on https://github.com/bitcoin/bitcoin/pull/31135

This has been tested on mainnet and seems to be working with the solution proposed:

```
bitcoin-cli -rpcport=8332 getblockchaininfo
{
"chain": "main",
"blocks": 881415,
"headers": 881415,
...
"verificationprogress": 1,
"initialblockdownload": false,
...
"warnings": [
"This is a pre-release test build - use at your own risk - do not use for mini
...

💬 achow101 commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2623151935)
> Not sure what you mean about breaking existing tooling though?

> I think it would break internal tooling not external tooling since this PR does not affect install or release locations. Earlier I suggested this could create symlinks in old locations for convenience and to prevent breakage. I don't think too many tools should actually break, but I do think this change could be annoying to developers, for example if you just update git and rebuild, and try to run the old binary because you do
...

💬 fjahr commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2623243798)
tACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

Reviewed the code and verified that the test change covers the new behavior.

> Did we have any blocks on mainnet in the last couple of years that would have violated the new mintime?

To answer my own question, I checked this with a small script and there were quite a few blocks before 400k that would have been below the new `mintime` but since then this has been a rare exception. Here is the full list of such blocks since 400k that I found (
...

💬 mzumsande commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934886294)
> I wonder if a good "mitigation" (if we're worried) is @mzumsande other suggestion, which is to process 1 thing off the peer's workset when it disconnects? Although if trying to "attack" this way, the solution then is to just have 2 in the workset.

Another idea that would deal with that is to assign it just to the peer who provided the parent here (no randomness), and during disconnection we don't do any validation but pass all oustanding workset entries on to another random announcer, if an
...

👍 tdb3 approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2582739849)
ACK efeaf2e0c1f0de0d7c3819eb9beaefee2ddf0710

💬 tdb3 commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1934897557)
Maybe it's worth migrating some of these to `p2p.py` as well?

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2623350369)
> > Not sure what you mean about breaking existing tooling though?
>
> I have several scripts which aid me in building and testing prs. These already have to figure out when to use autotools and when to use cmake. With this PR, these would now all need to figure out where the binaries live, and that seems to be significantly less trivial to do than detecting autotools or cmake.

I think it would be nice not just for tools but also for developers to just create symlinks from the old to new lo
...

💬 Aldocapurro commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1934939474)
En español porfavor

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934941277)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934941378)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934941489)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934941602)
done

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-2623406968)
Thanks for the review @hodlinator! comments addressed & added more details to the summary

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2623415052)
Rebased 5e0196c8e0d1687bcf59a9569bb8b7268d1f195d -> 0e4ee158cadc3eb8f6af1b33440ae9a95fe19487 ([`pr/wrap.17`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.17) -> [`pr/wrap.18`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.17-rebase..pr/wrap.18)) to fix conflict

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2623417011)
Updated 2a952710661a1990ed1c24b0ebd16de8ac0df87c -> 47a872236e070814ad74922d3f8a653e1c6af968 ([`pr/libexec.1`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.1) -> [`pr/libexec.2`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.1..pr/libexec.2)) to fix CI error

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1934978868)
> so why not create the `CTxOut` vector directly when we are creating the `scriptpubkeys` vector and then pass the `CTxOut` vector in as an argument?

I updated the PR to do that.

> I also think a comment explicitly mentioning we are creating an same number input, same number output transaction would be worth adding.

I also added a comment stating this

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2623477551)
I've updated the code to take a reference to the underlying test_setup object following @TheCharlatan and @josibake reviews. I've also added more comments explaining how the transactions in the test block are created.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2623504472)
> My first guess is the extra time is coming from hashing the transaction multiple times for the different signature digest algorithms? Just a guess; will need to dig into the code more to understand.

I'm pretty sure this is the case after looking in `interpreter.cpp`. I added a comment explaining this

👍 i-am-yuvi approved a pull request: "test: fixes p2p_ibd_txrelay wait time"
(https://github.com/bitcoin/bitcoin/pull/31759#pullrequestreview-2582919327)
ACK 1973a9e4f1dfba57051135d6e1bca80979de3879

Good catch!

```
p2p_ibd_txrelay.py | ✓ Passed | 2 s

ALL | ✓ Passed | 2 s (accumulated)
Runtime: 2 s

```