💬 glozow commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2110260609)
backport for 26.x in #29899
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2110260609)
backport for 26.x in #29899
👋 glozow's pull request is ready for review: "[26.x] archive 26.1 release notes + backports"
(https://github.com/bitcoin/bitcoin/pull/29899)
(https://github.com/bitcoin/bitcoin/pull/29899)
💬 naiyoma commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1600064674)
Thanks for the breakdown, seems ideal to also use satoshi_round here and to use round down for min and round up for max, helps with greater precision
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1600064674)
Thanks for the breakdown, seems ideal to also use satoshi_round here and to use round down for min and round up for max, helps with greater precision
💬 ajtowns commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2110275325)
> Another thing, it doesn't seem to exist an option to control the miner's internal "timer". That is, suppose I want to mine every minute instead of every 10 minutes.
The nbits options exist to control the miner's internal timer -- if you were to mine every minute instead of every 10 minutes, that will increase the difficulty until you run out of hashpower, at which point you'll be running your computer at full power 24/7 and end up getting blocks every 10 minutes. The "no matter how much has
...
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2110275325)
> Another thing, it doesn't seem to exist an option to control the miner's internal "timer". That is, suppose I want to mine every minute instead of every 10 minutes.
The nbits options exist to control the miner's internal timer -- if you were to mine every minute instead of every 10 minutes, that will increase the difficulty until you run out of hashpower, at which point you'll be running your computer at full power 24/7 and end up getting blocks every 10 minutes. The "no matter how much has
...
💬 sdaftuar commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1600064969)
Looks good, thanks!
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1600064969)
Looks good, thanks!
💬 sdaftuar commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2110283778)
re-utACK
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2110283778)
re-utACK
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600088583)
Oh nevermind, reviewing this PR gave me the answer: https://github.com/bitcoin/bitcoin/pull/29982
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600088583)
Oh nevermind, reviewing this PR gave me the answer: https://github.com/bitcoin/bitcoin/pull/29982
💬 naiyoma commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1600089403)
resolved
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1600089403)
resolved
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600099780)
Hmm I see, as per this comment it says "not dependent on any other transactions in the **mempool**" - does this also apply to other transactions in the **block**?
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L613
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600099780)
Hmm I see, as per this comment it says "not dependent on any other transactions in the **mempool**" - does this also apply to other transactions in the **block**?
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L613
👍 hebasto approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055510319)
ACK 850c73e250fe0b248cae80d26561da7047696e3d.
Thanks for the `test/util/check-deps.sh` script!
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055510319)
ACK 850c73e250fe0b248cae80d26561da7047696e3d.
Thanks for the `test/util/check-deps.sh` script!
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600117347)
Duplicated lines?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600117347)
Duplicated lines?
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600118750)
```suggestion
**Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto and util.
```
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600118750)
```suggestion
**Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto and util.
```
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600120289)
nit: Sort these lines?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600120289)
nit: Sort these lines?
💬 ryanofsky commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2110376536)
> Maybe I could start by having the `getblocktemplate` RPC use that new interface?
That's a good idea, nice way the interface could be used from the start.
> And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).
Right, you could look at the interfaces::Chain for an example of how to initialize the interface. Store the interface in NodeContext like:
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ec
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2110376536)
> Maybe I could start by having the `getblocktemplate` RPC use that new interface?
That's a good idea, nice way the interface could be used from the start.
> And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).
Right, you could look at the interfaces::Chain for an example of how to initialize the interface. Store the interface in NodeContext like:
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ec
...
💬 hebasto commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2110387585)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2110387585)
Concept ACK.
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110388198)
> Maybe this is starting to be more complicated than just having a struct per each reason? But I'd still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.
Same thing I was thinking. Using the class right now looks like it will make things more complicated. Maybe we should leave the class for a future change where it becomes necessary? @josibake @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110388198)
> Maybe this is starting to be more complicated than just having a struct per each reason? But I'd still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.
Same thing I was thinking. Using the class right now looks like it will make things more complicated. Maybe we should leave the class for a future change where it becomes necessary? @josibake @ryanofsky
👍 hebasto approved a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2055622041)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2055622041)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
💬 ryanofsky commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110436529)
> Seems easily addressed with a constructor, no? Something like:
Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition. Depending on the constructors it may also only provide runtime checking rather than compile-time checking like in your example. And if the struct is mutable could allow invalid representations of state after construction.
I don't know what is best in this particular case, I would ju
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110436529)
> Seems easily addressed with a constructor, no? Something like:
Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition. Depending on the constructors it may also only provide runtime checking rather than compile-time checking like in your example. And if the struct is mutable could allow invalid representations of state after construction.
I don't know what is best in this particular case, I would ju
...
💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110464755)
> Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition
Fair point, in that bugs could be introduced by someone not writing these pre-checks correctly / efficiently. I'll admit I'm not fully convinced that the struct per state approach isn't going to be harder to maintain / extend in the future, but given that I haven't convinced you guys on that point and you have convinced me there is an advantage per
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110464755)
> Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition
Fair point, in that bugs could be introduced by someone not writing these pre-checks correctly / efficiently. I'll admit I'm not fully convinced that the struct per state approach isn't going to be harder to maintain / extend in the future, but given that I haven't convinced you guys on that point and you have convinced me there is an advantage per
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600207532)
Thanks so much for the patch. I had to move this line back up to where it was, otherwise the test fails because after pushing the error into the batch reply, we would also then push a weird extra junk reply with the current id but the result from the last successful query
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 0e3e66c515..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -238,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPReque
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600207532)
Thanks so much for the patch. I had to move this line back up to where it was, otherwise the test fails because after pushing the error into the batch reply, we would also then push a weird extra junk reply with the current id but the result from the last successful query
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 0e3e66c515..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -238,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPReque
...