💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669167)
Ok, I dropped them for the commit and PR description and updated the code comment.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669229)
Renamed.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669273)
A failing `BOOST_REQUIRE` produces an easier to debug failure than dereferencing nullptr, so it's always a good one to add.

I updated the test to do this after every `miner->createNewBlock()` call.

Similarly `BOOST_REQUIRE` stops the test immedidately, so I replaced a few `BOOST_CHECK` calls with it.

💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569034227)
Rebased and addressed feedback.

💬 Sjors commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569056045)
> Could you clarify the effect of setting it to 4000 in light of the recent findings?

It's still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn't detect in the analysis.

In the example of Ocean it _seems_ that their custom `-blockmaxweight` is low en
...

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901686821)
> I don't think it would be possible to write a clang-tidy plugin to check for these cases

I haven't tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any `FunctionTemplateDecl` whose `FunctionDecl` does not have any `TemplateTypeParm` in the AST should be sufficient to find those that can be written without `template<...>`.

Example: https://godbolt.org/z/4rre4rPT6

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1901720687)
> The chainman reset may trigger validation interface notifications

Ah, right, `ChainStateFlushed` would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of `SyncWithValidationInterfaceQueue()` being called, so I suppose this doesn't really change anything wrt callbacks assuming `m_node.chainman` exists.

🚀 glozow merged a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121)

🤔 maflcko requested changes to a pull request: "ci: Switch to latest macOS and Windows images"
(https://github.com/bitcoin/bitcoin/pull/31597#pullrequestreview-2528979701)
not sure about this. Is there a reason to do this?

💬 maflcko commented on pull request "ci: Switch to latest macOS and Windows images":
(https://github.com/bitcoin/bitcoin/pull/31597#discussion_r1901724818)
Not sure about this. This should be the minimum version supported. IIRC xcode 15 corresponds to clang-16, somewhat?

If you want to test the latest xcode version, you can add a different task here, or in a nightly repo.

Otherwise, the build may break silently for xcode 15 and users/devs will complain.

🤔 l0rinc reviewed a pull request: "qa: Ensure consistent use of decimals instead of floats"
(https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2528976834)
I'm not sure about this, aren't we just masking a bigger problem (i.e. treating floats as if they were real numbers).
Also, can we update any one of the CI tasks to use the latest python to make sure we don't have to fix these manually all the time?

💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901727037)
is this really better than before?
```suggestion
wallet.sendtoaddress(address=address, amount=1)
```
or
```suggestion
wallet.sendtoaddress(address=address, amount="1")
```

Both seem to work

💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901722976)
Is it important this is a Decimal?
```suggestion
raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: "24.99"})
```
Seems to pass as well - floating point values weren't defined to be this precise anyway

💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901725082)
we have [many](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_signrawtransactionwithkey.py#L81) [other](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L291) [instances](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_bumpfee.py#L146) where we've kept the floating point format - can we set up a linter if that's important instead of fixing only a subset?

💬 maflcko commented on pull request "ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions":
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2569183122)
For reference, this conflicts with commit a1add80c80bc95afdd55652bb87379fb34150a5c from

> [#29881](https://github.com/bitcoin/bitcoin/pull/29881) (guix: use GCC 13 to build releases by fanquake)

However, this is fine, because any version 12, or above should be fine. I've edited the pull title to clarify GCC-12+.

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901746082)
https://en.cppreference.com/w/cpp/container/vector/emplace_back#:~:text=It%20demonstrates%20how%20emplace_back%20forwards%20parameters%20to%20the%20President%20constructor

This is handy!

🤔 rkrux reviewed a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2529015468)
Nit: In the PR description, a draft PR is mentioned as the project. Can add this link for project issue: https://github.com/bitcoin/bitcoin/issues/30392

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901757147)
Getting thrown off a bit by the name here. Is it really a histogram at this point or just a vector of fee rates/fracs?
Going by this def: https://en.wikipedia.org/wiki/Histogram

Sorry for not raising this earlier.

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901761122)
Might be helpful to add a comment here telling that the values in this vector are sorted. That's what I assumed while going through the `CalculatePercentiles` function here: https://github.com/bitcoin/bitcoin/pull/30157/files#diff-34fc4771c305399b6f7634acd8c5fd2d0b89115f998397d41676180d742c28daR10