💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644501633)
I would need to construct fully valid transaction for it to pass - since this doesn't have valid inputs it's considered a coinbase transaction which would need some extra data, which would be outside the scope of this validation. Since I'm only changing the outputs size and that triggers the correct error, I consider that to be enough.
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644501633)
I would need to construct fully valid transaction for it to pass - since this doesn't have valid inputs it's considered a coinbase transaction which would need some extra data, which would be outside the scope of this validation. Since I'm only changing the outputs size and that triggers the correct error, I consider that to be enough.
📝 instagibbs opened a pull request: "fuzz: have package_rbf always make small txns"
(https://github.com/bitcoin/bitcoin/pull/30300)
hopefully resolves https://github.com/bitcoin/bitcoin/issues/30241
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction "vsize", we can improve efficiency of the target
by explicitly creating very small transactionsm
reducing the hashing and mempory burden.
(https://github.com/bitcoin/bitcoin/pull/30300)
hopefully resolves https://github.com/bitcoin/bitcoin/issues/30241
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction "vsize", we can improve efficiency of the target
by explicitly creating very small transactionsm
reducing the hashing and mempory burden.
📝 theuni opened a pull request: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)
Drops two of our patches that have been merged upstream and adjusts the other to deal with recent changes.
Follow-up from #30283. I can't vouch for the upstream changes here.
(https://github.com/bitcoin/bitcoin/pull/30301)
Drops two of our patches that have been merged upstream and adjusts the other to deal with recent changes.
Follow-up from #30283. I can't vouch for the upstream changes here.
💬 instagibbs commented on issue "fuzz: timeout/oom in package_rbf":
(https://github.com/bitcoin/bitcoin/issues/30241#issuecomment-2176231802)
opened https://github.com/bitcoin/bitcoin/pull/30300, I think this should resolve it?
(https://github.com/bitcoin/bitcoin/issues/30241#issuecomment-2176231802)
opened https://github.com/bitcoin/bitcoin/pull/30300, I think this should resolve it?
💬 maflcko commented on pull request "fuzz: have package_rbf always make small txns":
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176233090)
> ConsumeTxMemPoolEntry generates its own transaction "vsize",
Can you add a reference to this claim? Looking at `src/test/fuzz/util/mempool.cpp`, I couldn't find it.
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176233090)
> ConsumeTxMemPoolEntry generates its own transaction "vsize",
Can you add a reference to this claim? Looking at `src/test/fuzz/util/mempool.cpp`, I couldn't find it.
💬 maflcko commented on pull request "fuzz: have package_rbf always make small txns":
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176238029)
Oh, I guess indirectly via `sig_op_cost`?
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176238029)
Oh, I guess indirectly via `sig_op_cost`?
🤔 fjahr reviewed a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2125652115)
tACK e977c698f97ac9bba30e4e3837f41721841c28c4
I reviewed the new changes and re-tested the behavior on Signet.
I mostly agree with @mzumsande above and will post some more thoughts on #30288 rather than here since the conversation seems to have moved over there.
So, since I agree this is not something we should encounter this is not a blocker, but just to confirm my understanding: with the current state of the change, if there was an actual other best chain deeper than the snapshot disc
...
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2125652115)
tACK e977c698f97ac9bba30e4e3837f41721841c28c4
I reviewed the new changes and re-tested the behavior on Signet.
I mostly agree with @mzumsande above and will post some more thoughts on #30288 rather than here since the conversation seems to have moved over there.
So, since I agree this is not something we should encounter this is not a blocker, but just to confirm my understanding: with the current state of the change, if there was an actual other best chain deeper than the snapshot disc
...
💬 fjahr commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1644544564)
nit: A bit easier to parse IMO, only if you have to retouch.
```suggestion
// When we sync with AssumeUtxo and discover the snapshot is not in the peer's best chain, abort:
```
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1644544564)
nit: A bit easier to parse IMO, only if you have to retouch.
```suggestion
// When we sync with AssumeUtxo and discover the snapshot is not in the peer's best chain, abort:
```
💬 maflcko commented on pull request "fuzz: have package_rbf always make small txns":
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176243342)
lgtm ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe
(Assuming my assumption about the vsize mocking via sigop cost is correct)
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176243342)
lgtm ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe
(Assuming my assumption about the vsize mocking via sigop cost is correct)
💬 instagibbs commented on pull request "fuzz: have package_rbf always make small txns":
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176244685)
> Oh, I guess indirectly via sig_op_cost?
Right, sorry!
https://github.com/bitcoin/bitcoin/pull/29879 previously touched on this
(https://github.com/bitcoin/bitcoin/pull/30300#issuecomment-2176244685)
> Oh, I guess indirectly via sig_op_cost?
Right, sorry!
https://github.com/bitcoin/bitcoin/pull/29879 previously touched on this
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1644607912)
I'm curious to know where do you get that parent count range from 🤔
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1644607912)
I'm curious to know where do you get that parent count range from 🤔
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2176353962)
Fixed the bug reported in https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2175491752. The problem was that the prevout index and the coins per txid are compact-size encoded (since #29612), but this script assumed varint-encoding. Since txs with a huge number of ouputs (>252) are rather rare, this apparently only pops up occassionally. Ready for review again.
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2176353962)
Fixed the bug reported in https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2175491752. The problem was that the prevout index and the coins per txid are compact-size encoded (since #29612), but this script assumed varint-encoding. Since txs with a huge number of ouputs (>252) are rather rare, this apparently only pops up occassionally. Ready for review again.
👋 theStack's pull request is ready for review: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432)
(https://github.com/bitcoin/bitcoin/pull/27432)
💬 ismaelsadeeq commented on issue "bumpfee doc about incrementalfee is incorrect":
(https://github.com/bitcoin/bitcoin/issues/29053#issuecomment-2176367468)
> Alternatively use the DEFAULT_INCREMENTAL_RELAY_FEE when fee_rate is set as suggested by @Sjors here: https://github.com/bitcoin/bitcoin/issues/26973
https://github.com/bitcoin/bitcoin/pull/27969 was merged, this can be closed?
(https://github.com/bitcoin/bitcoin/issues/29053#issuecomment-2176367468)
> Alternatively use the DEFAULT_INCREMENTAL_RELAY_FEE when fee_rate is set as suggested by @Sjors here: https://github.com/bitcoin/bitcoin/issues/26973
https://github.com/bitcoin/bitcoin/pull/27969 was merged, this can be closed?
💬 theStack commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2176371825)
Concept ACK 🚜
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2176371825)
Concept ACK 🚜
✅ maflcko closed an issue: "bumpfee doc about incrementalfee is incorrect"
(https://github.com/bitcoin/bitcoin/issues/29053)
(https://github.com/bitcoin/bitcoin/issues/29053)
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2176387707)
Rebased, squashed Mining interface usage into the existing commits and fixed the usage of `coinbase_output_max_additional_size`.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2176387707)
Rebased, squashed Mining interface usage into the existing commits and fixed the usage of `coinbase_output_max_additional_size`.
💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2176411143)
> if there was an actual other best chain deeper than the snapshot discovered, our node would get stuck because all other nodes would reorg to the best chain and then we would not request blocks from any of our peers, right?
yes, but as you mentioned only if all peers were on that other chain. Whereas the behavior on master would be to download some blocks and then crash with an assert trying to reorg to the new chain - note that this would also happen if the other chain was invalid (for reas
...
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2176411143)
> if there was an actual other best chain deeper than the snapshot discovered, our node would get stuck because all other nodes would reorg to the best chain and then we would not request blocks from any of our peers, right?
yes, but as you mentioned only if all peers were on that other chain. Whereas the behavior on master would be to download some blocks and then crash with an assert trying to reorg to the new chain - note that this would also happen if the other chain was invalid (for reas
...
👍 theStack approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2125877332)
re-ACK b2b4b932572c5bb1ffa3fc4f34e17130348fbc24
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2125877332)
re-ACK b2b4b932572c5bb1ffa3fc4f34e17130348fbc24
💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1644747936)
I would remove this line because it doesn't really add anything that the line below doesn't test.
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1644747936)
I would remove this line because it doesn't really add anything that the line below doesn't test.